-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
applications: nrf_desktop: Add support for nRF54L PDK (code only) #14364
applications: nrf_desktop: Add support for nRF54L PDK (code only) #14364
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
Test specificationCI/Jenkins/NRF
CI/Jenkins/integration
Detailed information of selected test modules Note: This message is automatically posted and updated by the CI |
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
applications/nrf_desktop/configuration/nrf54l15pdk_nrf54l15_cpuapp/led_state_def.h
Outdated
Show resolved
Hide resolved
|
||
CONFIG_LOG=y | ||
CONFIG_LOG_BACKEND_UART=y | ||
CONFIG_LOG_BACKEND_SHOW_COLOR=n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it by default "n"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nRF Desktop's logging configuration updates default of the CONFIG_LOG_BACKEND_SHOW_COLOR
if CONFIG_LOG_BACKEND_RTT
is enabled. I would set this Kconfig option here explicitly for consistency (and to avoid relying on default).
status = "disabled"; | ||
}; | ||
|
||
gpioled0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly that we cannot use default leds because we need to have one led per node?
Maybe do not disable but remove original leds node?
Also consider using default leds names: led0_g: led_0 -> led0: led_0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the reason behind disabling the node. The node is currently disabled (not removed) for other configuration so I would disable it also here for consistency.
I would prefer to stick to the current naming scheme too (also for consistency with existing GPIO LED configurations - e.g. nrf52833dk_nrf52820
).
6b62631
to
d6f9cb5
Compare
/* Redefine the partition map. */ | ||
&rram0 { | ||
/delete-node/ partitions; | ||
|
||
partitions { | ||
compatible = "fixed-partitions"; | ||
#address-cells = <1>; | ||
#size-cells = <1>; | ||
app_partition: partition@0 { | ||
label = "application"; | ||
reg = <0x0 0x177000>; | ||
}; | ||
|
||
storage_partition: partition@177000 { | ||
label = "storage"; | ||
reg = <0x177000 0x6000>; | ||
}; | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be considered: Maybe we could wait for your PM fix (#14368) and replace this DTS partition definitions with pm_static (if necessary)? It would be consistent then with the other targets :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I would merge it as is and then switch to Partition Manager together with MCUboot integration. I guess we will do it soon and that would prevent from freezing code on PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, if no one will object then let's leave it for now as it is.
d6f9cb5
to
8b3c77a
Compare
8b3c77a
to
df8f3da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just left some informational comments below
# GPIOs assigned to Button 3 and Button 4 on the PDK v0.2.1 do not support interrupts. | ||
# Because of this, the GPIOs are not supported by the CAF buttons module and cannot be used to | ||
# generate motion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to have a way to distinguish between v0.2.x PDKs and v0.3.0. However, this is a topic for subsequent PR when the v0.3 support is ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require a new approach globally
static const struct gpio_pin row[] = { | ||
{ .port = 1, .pin = DT_GPIO_PIN(DT_NODELABEL(button0), gpios) }, | ||
{ .port = 1, .pin = DT_GPIO_PIN(DT_NODELABEL(button1), gpios) }, | ||
/* GPIOs assigned to Button 3 and Button 4 on the PDK v0.2.1 do not support interrupts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
CONFIG_DESKTOP_MOTION_BUTTONS_ENABLE=y | ||
CONFIG_DESKTOP_MOTION_BUTTONS_LEFT_KEY_ID=0 | ||
CONFIG_DESKTOP_MOTION_BUTTONS_RIGHT_KEY_ID=1 | ||
# GPIOs assigned to Button 3 and Button 4 on the PDK v0.2.1 do not support interrupts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here :)
applications/nrf_desktop/configuration/nrf54l15pdk_nrf54l15_cpuapp/prj.conf
Outdated
Show resolved
Hide resolved
The PDK can act as either keyboard or mouse. Jira: NCSDK-26216 Signed-off-by: Marek Pieta <[email protected]> Signed-off-by: Pirun Lee <[email protected]>
df8f3da
to
63b1040
Compare
The PDK can act as either keyboard or mouse. The PR also brings in watchdog alignment.
Jira: NCSDK-26216