devices: make device name field optional #49352

gmarull · 2022-08-22T14:00:57Z

With the adoption of DEVICE_DT_GET() device names are in many cases
optional and used only for stuff like debugging or in shells. This patch
makes struct device name field optional so that ROM can be saved in case
user can afford it. This means that the name field in struct device
should no longer be accessed directly but using a new function:
device_name_get().

Note that the option controlling such feature is
CONFIG_DEVICE_STORE_NAME, enabled by default.

Some quick numbers using hello_world:

nrf52840dk_nrf52840: $\Delta_{\textrm{FLASH}} = -100~\textrm{bytes}$
nucleo_h743zi: $\Delta_{\textrm{FLASH}} = -272~\textrm{bytes}$

Signed-off-by: Gerard Marull-Paretas [email protected]

galak

I'd suggest maybe splitting this PR as the device_name_get cleanup could get merged now, and than we can hash out the testcase and details on CONFIG_DEVICE_STORE_NAME

MaureenHelm · 2022-08-24T14:18:46Z

Sensor drivers and samples look good.
We need a test that builds with CONFIG_DEVICE_STORE_NAME=n

any suggestion? not sure where to test this

Maybe start with samples/basic/minimal? It's not exactly thorough, but it would be better than nothing.

gmarull · 2022-08-24T14:26:13Z

I'd suggest maybe splitting this PR as the device_name_get cleanup could get merged now, and than we can hash out the testcase and details on CONFIG_DEVICE_STORE_NAME

you mean adding the API without the optional name field changes?

galak · 2022-08-24T14:54:48Z

you mean adding the API without the optional name field changes?

yes. Since the 95% of this PR is the API and changes to use it. I think its reasonable to encapsulate the access to name.

drivers/flash/flash_shell.c

mbolivar-nordic · 2022-08-26T18:55:57Z

include/zephyr/device.h

+#ifdef CONFIG_DEVICE_STORE_NAME
+	return dev->name;
+#else
+	return "";


It might make sense to allocate a global string in rom with value "<device>" or something like that, and return a pointer to it here. Otherwise log messages that print device names will be kind of busted with CONFIG_DEVICE_STORE_NAME=n in my opinion.

adjusted to return "<device>"

mbolivar-nordic · 2022-08-26T19:01:16Z

include/zephyr/device.h

+{
+	ARG_UNUSED(name);
+
+	return NULL;


This feels like a footgun implementation.

I believe we should not provide a device_get_binding implementation at all, at least by default, if CONFIG_DEVICE_STORE_NAME=n.

But I get why you would want this, so perhaps it should be configurable? Something like...

choice prompt "device_get_binding() behavior" depends on !DEVICE_STORE_NAME config NO_DEVICE_GET_BINDING bool "No implementation" help No device_get_binding() implementation will be provided. Use of this function will cause build errors. config NULL_DEVICE_GET_BINDING bool "NULL implementation" help A stub device_get_binding() implementation will be provided that returns NULL. endchoice

with NO_DEVICE_GET_BINDING being the default behavior

We discussed this yesterday in dev-review, agreement was to disable device_get_binding completely, so you'll get a build failure if you can't really use this option.

mbolivar-nordic · 2022-08-26T19:02:42Z

kernel/Kconfig

@@ -951,6 +951,14 @@ config HAS_DYNAMIC_DEVICE_HANDLES
 	  Hidden option that makes possible to manipulate device handles at
 	  runtime.

+config DEVICE_STORE_NAME


Seems my above concern is already discussed here; I agree with @galak on this one, at least for the default.

gmarull · 2022-09-02T07:59:42Z

Sensor drivers and samples look good.
We need a test that builds with CONFIG_DEVICE_STORE_NAME=n

any suggestion? not sure where to test this

Maybe start with samples/basic/minimal? It's not exactly thorough, but it would be better than nothing.

disabled the option in this sample

mbolivar-nordic

I'm all for the dev-review solution. One suggested doc improvement, but this looks good for v3.3 to me.

doc/develop/optimizations/footprint.rst

include/zephyr/device.h

Rename the function to avoid clashed with the upcoming device_name_get. Signed-off-by: Gerard Marull-Paretas <[email protected]>

sd_gpio was checked for NULL using its name, so the condition was always true. Check if the port (device) itself is NULL instead. Signed-off-by: Gerard Marull-Paretas <[email protected]>

With the adoption of DEVICE_DT_GET() device names are in many cases optional and used only for stuff like debugging or in shells. This patch makes struct device name field optional so that ROM can be saved in case user can afford it. This means that the name field in struct device should no longer be accessed directly but using a new function: device_name_get(). Note that the option controlling such feature is CONFIG_DEVICE_STORE_NAME, enabled by default. Signed-off-by: Gerard Marull-Paretas <[email protected]>

Since struct device name field is now optional, use device_name_get. Signed-off-by: Gerard Marull-Paretas <[email protected]>

CONFIG_DEVICE_STORE_NAME has the potential to reduce ROM usage in case application images do not make use of device_get_binding(), so mention it in the footprint documentation. Signed-off-by: Gerard Marull-Paretas <[email protected]>

Minimal sample is meant to show the minimum Zephyr ROM footprint. Disable CONFIG_DEVICE_STORE_NAME since the option targets ROM footprint reduction as well. Signed-off-by: Gerard Marull-Paretas <[email protected]>

github-actions · 2022-11-21T00:30:18Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

galak added the area: Device Model label Aug 22, 2022

gmarull force-pushed the device-name-optional branch 3 times, most recently from cee3e2c to d4c3e34 Compare August 22, 2022 15:14

gmarull mentioned this pull request Aug 22, 2022

Refining Zephyr's Device Driver Model - Take II #22941

Open

13 tasks

gmarull force-pushed the device-name-optional branch 5 times, most recently from 314707e to b9aef90 Compare August 23, 2022 10:14

gmarull marked this pull request as ready for review August 23, 2022 10:15

gmarull requested review from andyross, nashif, mnkp, henrikbrixandersen, sjg20, nordic-krch, alexanderwachter, jfischer-no, nvlsianpu, jakub-uC, dleach02, ceolin, rlubos, tbursztyka, pfalcon, sidcha, nandojve, avisconti and MaureenHelm as code owners August 23, 2022 10:15

gmarull force-pushed the device-name-optional branch from 751b784 to 9f10be7 Compare August 24, 2022 13:48

galak reviewed Aug 24, 2022

View reviewed changes

ibirnbaum mentioned this pull request Aug 25, 2022

drivers: can: Xilinx Processor System CAN controller driver #45101

Closed

gmarull added the dev-review To be discussed in dev-review meeting label Aug 25, 2022

mbolivar-nordic reviewed Aug 26, 2022

View reviewed changes

gmarull force-pushed the device-name-optional branch from 9f10be7 to de2cc84 Compare September 2, 2022 07:56

gmarull requested a review from galak September 2, 2022 08:21

MaureenHelm removed the dev-review To be discussed in dev-review meeting label Sep 7, 2022

mbolivar-nordic reviewed Sep 14, 2022

View reviewed changes

doc/develop/optimizations/footprint.rst Outdated Show resolved Hide resolved

gmarull force-pushed the device-name-optional branch from de2cc84 to ac83ac3 Compare September 19, 2022 08:53

gmarull requested a review from mateusz-holenko as a code owner September 19, 2022 08:53

gmarull removed the request for review from mateusz-holenko September 19, 2022 08:54

gmarull added this to the v3.3.0 milestone Sep 19, 2022

nvlsianpu reviewed Sep 20, 2022

View reviewed changes

include/zephyr/device.h Show resolved Hide resolved

include/zephyr/device.h Outdated Show resolved Hide resolved

gmarull added 2 commits September 21, 2022 11:25

drivers: shells: s/device_name_get/cmd_device_name_get

f08b1ca

Rename the function to avoid clashed with the upcoming device_name_get. Signed-off-by: Gerard Marull-Paretas <[email protected]>

drivers: sensor: fdc2x1x: fix device NULL check

0e6261a

sd_gpio was checked for NULL using its name, so the condition was always true. Check if the port (device) itself is NULL instead. Signed-off-by: Gerard Marull-Paretas <[email protected]>

gmarull force-pushed the device-name-optional branch from ac83ac3 to 1fecfd6 Compare September 21, 2022 09:25

gmarull added 4 commits September 21, 2022 12:04

devices: use device_name_get

b36e67f

Since struct device name field is now optional, use device_name_get. Signed-off-by: Gerard Marull-Paretas <[email protected]>

samples: basic: minimal: disable CONFIG_DEVICE_STORE_NAME

f5a2033

Minimal sample is meant to show the minimum Zephyr ROM footprint. Disable CONFIG_DEVICE_STORE_NAME since the option targets ROM footprint reduction as well. Signed-off-by: Gerard Marull-Paretas <[email protected]>

gmarull force-pushed the device-name-optional branch from 1fecfd6 to f5a2033 Compare September 21, 2022 10:04

nvlsianpu approved these changes Sep 21, 2022

View reviewed changes

github-actions bot added the Stale label Nov 21, 2022

github-actions bot closed this Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

devices: make device name field optional #49352

devices: make device name field optional #49352

Uh oh!

gmarull commented Aug 22, 2022 •

edited

Loading

Uh oh!

galak left a comment

Uh oh!

MaureenHelm commented Aug 24, 2022

Uh oh!

gmarull commented Aug 24, 2022

Uh oh!

galak commented Aug 24, 2022

Uh oh!

Uh oh!

mbolivar-nordic Aug 26, 2022

Uh oh!

gmarull Sep 2, 2022

Uh oh!

mbolivar-nordic Aug 26, 2022

Uh oh!

gmarull Sep 2, 2022

Uh oh!

mbolivar-nordic Aug 26, 2022

Uh oh!

gmarull commented Sep 2, 2022

Uh oh!

mbolivar-nordic left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 21, 2022

Uh oh!

Uh oh!

devices: make device name field optional #49352

devices: make device name field optional #49352

Uh oh!

Conversation

gmarull commented Aug 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

MaureenHelm commented Aug 24, 2022

Uh oh!

gmarull commented Aug 24, 2022

Uh oh!

galak commented Aug 24, 2022

Uh oh!

Uh oh!

mbolivar-nordic Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

gmarull Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

mbolivar-nordic Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

gmarull Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

mbolivar-nordic Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

gmarull commented Sep 2, 2022

Uh oh!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 21, 2022

Uh oh!

Uh oh!

gmarull commented Aug 22, 2022 •

edited

Loading