Skip to content
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

samples/sensor/*: Build issue when board expose sensors defined on both I2C and SPI buses #48518

Closed
erwango opened this issue Aug 1, 2022 · 7 comments · Fixed by #48707
Closed
Assignees
Labels
area: Devicetree area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@erwango
Copy link
Member

erwango commented Aug 1, 2022

Describe the bug
There are a few sensor samples which fail to build with 96b_argonkey:
samples/sensor/vl53l0x/
samples/sensor/hts221
samples/sensor/lps22hb

/<redacted>/zephyr-sdk/sdk-0.14.0/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/10.3.0/../../../../arm-zephyr-eabi/bin/ld.bfd: zephyr/drivers/sensor/lsm6dsl/libdrivers__sensor__lsm6dsl.a(lsm6dsl.c.obj):(.rodata.lsm6dsl_config_0+0x4): undefined reference to `__device_dts_ord_83'

Root cause is:

  • Board dts defines lsm6dsl on SPI bus, while other sensors (hts221, vl53l0x, lps22hb) are defined on I2C bus
  • samples for I2C sensors enable both CONFIG_SENSOR and CONFIG_I2C
  • Due to CONFIG_SENSOR=y drivers/sensor/lsm6dsl.c is compiled, but compilation fails due to CONFIG_SPI not being enabled.

This is likely an effect of:

menuconfig LSM6DSL
	bool "LSM6DSL I2C/SPI accelerometer and gyroscope Chip"
	default y
+	depends on DT_HAS_ST_LSM6DSL_ENABLED
	depends on I2C || SPI

Ideally LSM6DSL activation should be a little bit more selective and check bus activation:

	depends on DT_HAS_ST_LSM6DSL_ENABLED
	depends on ((I2C && DT_LSM6DSL_ENABLED_ON_I2C_BUS) || 
                   (SPI && DT_LSM6DSL_ENABLED_ON_SPI_BUS))
@erwango erwango added the bug The issue is a bug, or the PR is fixing a bug label Aug 1, 2022
@erwango erwango added area: Sensors Sensors priority: medium Medium impact/importance bug area: Devicetree priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Aug 1, 2022
@galak
Copy link
Collaborator

galak commented Aug 2, 2022

@erwango I had worked up something similar to what you suggested in a PR:

#48048

depends on (I2C && $(dt_compat_on_bus,$(DT_COMPAT_ADI_ADXL345),i2c)) || \
		   (SPI && $(dt_compat_on_bus,$(DT_COMPAT_ADI_ADXL345),spi))

However @MaureenHelm asked to just update the board Kconfig.defconfig to enable the bus needed. So will look for her comment on this to see what direction we should go.

@erwango
Copy link
Member Author

erwango commented Aug 2, 2022

@MaureenHelm asked to just update the board Kconfig.defconfig to enable the bus needed

If this is something like follows, this would make sense indeed:

config SPI
    default y if LSM6DSL

config I2C
   default y if VL53L0X || HTS221 || LPS22HB

Edit: There would be a consequence on samples/sensors/*/prj.conf since enabling SENSOR would then automatically bring up buses. So CONFIG_I2C=y and CONFIG_SPI=y would have to be cleaned up.
Another consequence is that in case of board has sensor on both bus. Enabling SENSOR would bring up both buses automatically. But this could be fine tuned using .overlays, so that is not a concern to me.

Then, no reason to go to the next step which would be to get some DT magic to do this automatically

@MaureenHelm
Copy link
Member

depends on (I2C && $(dt_compat_on_bus,$(DT_COMPAT_ADI_ADXL345),i2c)) || \
		   (SPI && $(dt_compat_on_bus,$(DT_COMPAT_ADI_ADXL345),spi))

I really don't like this pattern.

If this is something like follows, this would make sense indeed:

config SPI
    default y if LSM6DSL

config I2C
   default y if VL53L0X || HTS221 || LPS22HB

I tried this before, but it created a circular dependency (LSM6DSL depends on SPI, and SPI depends on LSM6DSL). That's why I wound up using SENSOR instead of LSM6DSL.

config SPI
	default y if SENSOR

Edit: There would be a consequence on samples/sensors/*/prj.conf since enabling SENSOR would then automatically bring up buses. So CONFIG_I2C=y and CONFIG_SPI=y would have to be cleaned up. Another consequence is that in case of board has sensor on both bus. Enabling SENSOR would bring up both buses automatically. But this could be fine tuned using .overlays, so that is not a concern to me.

Agreed

@galak
Copy link
Collaborator

galak commented Aug 4, 2022

@erwango will you work up a PR for this?

@erwango
Copy link
Member Author

erwango commented Aug 4, 2022

That's why I wound up using SENSOR instead of LSM6DSL.

config SPI
	default y if SENSOR

I'm not convinced by this solution. This enables bus irrespective of sensor devicetree status on the bus.
We end up in a situation where:

  • If I have sensors on one type of bus, bus is activated only if required, based on devices device tree status.
  • If I have sensors splits on 2 types of bus, both bus are activated irrespective of devicetree status (as long as one sensor is enabled). If I want to enable sensors on one type of bus, I also need to manually edit board_config to disable the bus not needed, this cannot be fined tuned only by .overlays.

So I kind of prefer solution proposed here

@erwango
Copy link
Member Author

erwango commented Aug 4, 2022

Dev meeting: Review the bus selection: select bus in drivers and remove board related code.
I'll have a try and propose a PR

@galak
Copy link
Collaborator

galak commented Aug 4, 2022

I'll have a try and propose a PR

pushed a PR, will see how it goes through CI.

galak added a commit to galak/zephyr that referenced this issue Aug 5, 2022
This change in pattern is meant to address a misconfiguration issue
that can occur for sensors that support being on multiple busses
like I2C & SPI.

For example, you can have a configuration in which such a sensor is
on the I2C bus in the devicetree and the sensor is enabled.  However
the application configuration enables CONFIG_SPI=y and CONFIG_I2C=n
and this will cause the sensor driver to be built by default, however
since we don't have the I2C bus enabled the driver will not compile
correctly.

Previously we had been adding to board Kconfig.defconfig something
like:

	config I2C
		default y if SENSOR

This pattern doesn't scale well and may differ from what an application
specific need/use is.

So instead move to a pattern in which we leave the default enablement
up to the devicetree "status" property for the sensor.  We then have
the Kconfig move from 'depends on <BUS>' to 'select <BUS>' and in
the case of drivers that support multiple busses we have the Kconfig
be: 'select <BUS> if $(dt_compat_on_bus,$(<DT_COMPAT>),<BUS>) for
each bus type the sensor supports.

This removes the need to add Kconfig logic to each board and enables
the bus subsystem and bus controller driver if the sensor requires
it by default in the build system.

Fixes: zephyrproject-rtos#48518

Signed-off-by: Kumar Gala <galak@kernel.org>
fabiobaltieri pushed a commit that referenced this issue Aug 8, 2022
This change in pattern is meant to address a misconfiguration issue
that can occur for sensors that support being on multiple busses
like I2C & SPI.

For example, you can have a configuration in which such a sensor is
on the I2C bus in the devicetree and the sensor is enabled.  However
the application configuration enables CONFIG_SPI=y and CONFIG_I2C=n
and this will cause the sensor driver to be built by default, however
since we don't have the I2C bus enabled the driver will not compile
correctly.

Previously we had been adding to board Kconfig.defconfig something
like:

	config I2C
		default y if SENSOR

This pattern doesn't scale well and may differ from what an application
specific need/use is.

So instead move to a pattern in which we leave the default enablement
up to the devicetree "status" property for the sensor.  We then have
the Kconfig move from 'depends on <BUS>' to 'select <BUS>' and in
the case of drivers that support multiple busses we have the Kconfig
be: 'select <BUS> if $(dt_compat_on_bus,$(<DT_COMPAT>),<BUS>) for
each bus type the sensor supports.

This removes the need to add Kconfig logic to each board and enables
the bus subsystem and bus controller driver if the sensor requires
it by default in the build system.

Fixes: #48518

Signed-off-by: Kumar Gala <galak@kernel.org>
PavelVPV pushed a commit to PavelVPV/zephyr that referenced this issue Aug 23, 2022
…select'

This change in pattern is meant to address a misconfiguration issue
that can occur for sensors that support being on multiple busses
like I2C & SPI.

For example, you can have a configuration in which such a sensor is
on the I2C bus in the devicetree and the sensor is enabled.  However
the application configuration enables CONFIG_SPI=y and CONFIG_I2C=n
and this will cause the sensor driver to be built by default, however
since we don't have the I2C bus enabled the driver will not compile
correctly.

Previously we had been adding to board Kconfig.defconfig something
like:

	config I2C
		default y if SENSOR

This pattern doesn't scale well and may differ from what an application
specific need/use is.

So instead move to a pattern in which we leave the default enablement
up to the devicetree "status" property for the sensor.  We then have
the Kconfig move from 'depends on <BUS>' to 'select <BUS>' and in
the case of drivers that support multiple busses we have the Kconfig
be: 'select <BUS> if $(dt_compat_on_bus,$(<DT_COMPAT>),<BUS>) for
each bus type the sensor supports.

This removes the need to add Kconfig logic to each board and enables
the bus subsystem and bus controller driver if the sensor requires
it by default in the build system.

Fixes: zephyrproject-rtos#48518

Signed-off-by: Kumar Gala <galak@kernel.org>
(cherry picked from commit df81fef)
nordic-krch pushed a commit to nordic-krch/zephyr that referenced this issue Sep 8, 2022
…select'

This change in pattern is meant to address a misconfiguration issue
that can occur for sensors that support being on multiple busses
like I2C & SPI.

For example, you can have a configuration in which such a sensor is
on the I2C bus in the devicetree and the sensor is enabled.  However
the application configuration enables CONFIG_SPI=y and CONFIG_I2C=n
and this will cause the sensor driver to be built by default, however
since we don't have the I2C bus enabled the driver will not compile
correctly.

Previously we had been adding to board Kconfig.defconfig something
like:

	config I2C
		default y if SENSOR

This pattern doesn't scale well and may differ from what an application
specific need/use is.

So instead move to a pattern in which we leave the default enablement
up to the devicetree "status" property for the sensor.  We then have
the Kconfig move from 'depends on <BUS>' to 'select <BUS>' and in
the case of drivers that support multiple busses we have the Kconfig
be: 'select <BUS> if $(dt_compat_on_bus,$(<DT_COMPAT>),<BUS>) for
each bus type the sensor supports.

This removes the need to add Kconfig logic to each board and enables
the bus subsystem and bus controller driver if the sensor requires
it by default in the build system.

Fixes: zephyrproject-rtos#48518

Signed-off-by: Kumar Gala <galak@kernel.org>
(cherry picked from commit 550a80a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants