rtc: shell: support node labels and allow only rtc devices #82219
Conversation
yishai1999
force-pushed
the
rtc_shell_improvements
branch
from
6dce7ff to
94cf13b
Compare
|
Hi, you are in luck! We just merged a new feature in zephyr which allows us to iterate devices by API type, an implementation of it is already ready for comparators, see #82186 and I'm working on updating the RTC drivers and shell to use it as well :) |
|
The new feature does not allow one to iterate on node labels though, which I thinks is probably okay given nodes can have multiple node labels, making it a bit complex to filter on |
yishai1999
force-pushed
the
rtc_shell_improvements
branch
from
94cf13b to
b312008
Compare
pdgendt
left a comment
pdgendt
left a comment
There was a problem hiding this comment.
NAK, in favor of the suggestion by @bjarki-andreasen
It's fine that the autocompletion won't detect the node labels but maybe it can still accept them by using |
Added support for using node labels instead of only full node name Signed-off-by: Yishai Jaffe <[email protected]>
yishai1999
force-pushed
the
rtc_shell_improvements
branch
from
b312008 to
285e238
Compare
bjarki-andreasen
left a comment
bjarki-andreasen
left a comment
There was a problem hiding this comment.
Nice :) Did not know device_get_by_dt_nodelabel() existed :)
It's new! |
fabiobaltieri
left a comment
fabiobaltieri
left a comment
There was a problem hiding this comment.
Not loving this to be totally honest, first I also don't think it's a great idea to start sprinkling imply DEVICE_DT_METADATA around, but also this adds a "device_get_by_name_or_nodelabel" at rtc subsystem level... it should really just be a device API level function that can be used by multiple subsystems, there's nothing rtc specific about it.
| /* Look up a device by some human-readable string identifier. We | ||
| * always search among device names. If the feature is available, we | ||
| * search by node label as well. | ||
| */ | ||
| static const struct device *get_rtc_device(char *id) | ||
| { | ||
| const struct device *dev; | ||
|
|
||
| dev = device_get_binding(id); | ||
| if (dev != NULL) { | ||
| return dev; | ||
| } | ||
|
|
||
| #ifdef CONFIG_DEVICE_DT_METADATA | ||
| dev = device_get_by_dt_nodelabel(id); | ||
| if (dev != NULL) { | ||
| return dev; | ||
| } | ||
| #endif /* CONFIG_DEVICE_DT_METADATA */ | ||
|
|
||
| return NULL; | ||
| } |
There was a problem hiding this comment.
why don't we add a shell-level API to obtain a device pointer using either dev name/nodelabel id?
There was a problem hiding this comment.
yeah that was my point too
There was a problem hiding this comment.
sounds good! And then imply CONFIG_DEVICE_DT_METADATA in the shell Kconfig?
There was a problem hiding this comment.
As it stands, placed under a specific subsystem, it looks a bit off to me. We'd end up with the option auto enabling or not by default depending on some random combination of what subsystem are enabled, and what the corresponding contributors and maintainers decided.
If you really want to make it more exposed to random users, how about changing it to default y if !SHELL_MINIMAL? Then it would be coupled with the SHELL subsystem, which makes more sense to me.
There was a problem hiding this comment.
cc @henrikbrixandersen who IIRC cleaned up a bunch of SHELL options to solve a similar problems (incoherent defaults)
There was a problem hiding this comment.
We may even be able to extend the common API to autocomplete device names/nodelabels for a given device API type.
There was a problem hiding this comment.
We may even be able to extend the common API to autocomplete device names/nodelabels for a given device API type.
Yeah seen some first of those patches in the wild already 1c5c28b
There was a problem hiding this comment.
Wouldn't adding default y if !SHELL_MINIMAL to CONFIG_DEVICE_DT_METADATA be kind of weird since CONFIG_DEVICE_DT_METADATA is under the kernel Kconfig? This would be the first mention of SHELL_MINIMAL outside of shell related Kconfigs
There was a problem hiding this comment.
Wouldn't adding
default y if !SHELL_MINIMALtoCONFIG_DEVICE_DT_METADATAbe kind of weird sinceCONFIG_DEVICE_DT_METADATAis under thekernelKconfig? This would be the first mention ofSHELL_MINIMALoutside of shell related Kconfigs
yup, sounds like we should instead have something like:
config SHELL_DEVICE_HELPERS
bool "Shell device helpers"
imply DEVICE_DT_METADATA
default y if !SHELL_MINIMAL
There was a problem hiding this comment.
Ok, I opened a new PR (#82698) to add this. It would be great if you all could take a look.
7 participants
Added support for using node labels instead of only full node name and added a verification that the given device is an rtc device.