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

[BUG] CONFIG_DEBUG_UORB should enable CONFIG_LIBC_FLOATINGPOINT #15599

Open
1 task done
linguini1 opened this issue Jan 19, 2025 · 19 comments
Open
1 task done

[BUG] CONFIG_DEBUG_UORB should enable CONFIG_LIBC_FLOATINGPOINT #15599

linguini1 opened this issue Jan 19, 2025 · 19 comments
Labels
Arch: all Issues that apply to all architectures Area: Configuring Configuring issues OS: Linux Issues related to Linux (building system, etc) Type: Bug Something isn't working

Comments

@linguini1
Copy link
Contributor

Description / Steps to reproduce the issue

When enabling the UORB debug output, I believe that printing floating point numbers in printf should also be selected and enabled.

Reasoning: All (or the vast majority) of UORB data types are float based. If someone is enabling UORB debugging, they most likely want to see these measurements printed. It's very easy to forget that printing floats isn't enabled by default and requires rebuilding and re-flashing to correct. This can get tedious, even if only selecting the option in menuconfig.

Having this be auto-selected brings NuttX debugging a step closer to "just working", or being less tedious anyways.

Steps to reproduce:

  • Enable DEBUG_UORB in menuconfig
  • Enable one of the UORB sensors (i.e SHT4X)
  • Boot this image and run uorb_listener in NSH
  • Observe the below
NuttShell (NSH) NuttX-12.8.0
 
nsh> 
nsh> uorb_listener

Mointor objects num:3
object_name:sensor_temp, object_instance:3
object_name:sensor_temp, object_instance:2
object_name:sensor_temp, object_instance:1
sensor_temp(now:164480000):timestamp:164480000,temperature:*float*
sensor_temp(now:164480000):timestamp:164480000,temperature:*float*
sensor_temp(now:164480000):timestamp:164480000,temperature:*float*
sensor_temp(now:165490000):timestamp:165490000,temperature:*float*
sensor_temp(now:165490000):timestamp:165490000,temperature:*float*
sensor_temp(now:165490000):timestamp:165490000,temperature:*float*
sensor_temp(now:166500000):timestamp:166500000,temperature:*float*
sensor_temp(now:166500000):timestamp:166500000,temperature:*float*

Caveats:

I know there is some finickiness with the Kconfig select keyword, if there are suggestions to do this without breaking dependency structuring that might be good. I think depends on is not the correct approach because I believe it will hide the DEBUG_UORB option until LIBC_FLOATING_POINT is enabled first.

On which OS does this issue occur?

[OS: Linux]

What is the version of your OS?

Arch Linux

NuttX Version

12.8.0

Issue Architecture

[Arch: all]

Issue Area

[Area: Configuring]

Verification

  • I have verified before submitting the report.
@linguini1 linguini1 added the Type: Bug Something isn't working label Jan 19, 2025
@github-actions github-actions bot added Arch: all Issues that apply to all architectures Area: Configuring Configuring issues OS: Linux Issues related to Linux (building system, etc) labels Jan 19, 2025
@acassis
Copy link
Contributor

acassis commented Jan 20, 2025

@linguini1 since uORB needs float, maybe when some uORB sensor is enabled it should enable CONFIG_LIBC_FLOATINGPOINT by default.

What do you think @xiaoxiang781216 ?

@raiden00pl
Copy link
Member

raiden00pl commented Jan 20, 2025

only CONFIG_DEBUG_UORB needs CONFIG_LIBC_FLOATINGPOINT. So why not add simple error in apps/system/uorb/uORB/uORB.c:

#if defined(CONFIG_DEBUG_UORB) && !defined(CONFIG_LIBC_FLOATINGPOINT)
#  error blah blah blah
#endif

@acassis
Copy link
Contributor

acassis commented Jan 20, 2025

@raiden00pl good point! But the sensors are using float by default, so soon or later user will want to printf the result and that will create frustration for them and support for us.

I think CONFIG_LIBC_FLOATINGPOINT is something that should be enabled by default always, execpt maybe for MCUs that doesn't have FPU or SMALL is enabled.

@raiden00pl
Copy link
Member

@raiden00pl good point! But the sensors are using float by default, so soon or later user will want to printf the result and that will create frustration for them and support for us.

I can't agree with that. You print sensors values only when debugging or developing your app. In real life applications printing the sensor value is probably not what you want to achieve, but process the value in a different way.

I think CONFIG_LIBC_FLOATINGPOINT is something that should be enabled by default always, execpt maybe for MCUs that doesn't have FPU or SMALL is enabled.

That's exactly how it is:

config LIBC_FLOATINGPOINT
	bool "Enable floating point in printf"
	default !DEFAULT_SMALL && ARCH_FPU
	depends on !LIBM_NONE

@acassis
Copy link
Contributor

acassis commented Jan 20, 2025

@

@raiden00pl good point! But the sensors are using float by default, so soon or later user will want to printf the result and that will create frustration for them and support for us.

I can't agree with that. You print sensors values only when debugging or developing your app. In real life applications printing the sensor value is probably not what you want to achieve, but process the value in a different way.

I think CONFIG_LIBC_FLOATINGPOINT is something that should be enabled by default always, execpt maybe for MCUs that doesn't have FPU or SMALL is enabled.

That's exactly how it is:

config LIBC_FLOATINGPOINT
	bool "Enable floating point in printf"
	default !DEFAULT_SMALL && ARCH_FPU
	depends on !LIBM_NONE

Ouch! So it is already fine! My fault, I should have double checked before adding my statement.

@linguini1 I think just including the #error "You need to enable CONFIG_LIBC_FLOATINGPOINT to get UORB DEBUG working" is enough in this case.

@raiden00pl
Copy link
Member

@acassis if I remember correctly @linguini1 works with rp2040, which is Cortex M0+ and has no FPU support (ARCH_FPU=n).

@linguini1
Copy link
Contributor Author

That's right, I'm mostly working with RP2040 based boards but I will be working with the STM32H7 shortly once InSpace's flight computer is built. I agree with the stance that floats in printf only need to be enabled when debugging UORB, not for regular use. I was hoping that there would be some way to enable both when UORB_DEBUG gets enabled, but an error message is definitely better than nothing so I can go ahead and add that.

@acassis
Copy link
Contributor

acassis commented Jan 20, 2025

@acassis if I remember correctly @linguini1 works with rp2040, which is Cortex M0+ and has no FPU support (ARCH_FPU=n).

Yes, in this case is better to just advice the user to enable support in menuconfig, because float support will be implemented by software (from toolchain libc)

@xiaoxiang781216
Copy link
Contributor

Do you enable DEBUG_UORB, which auto select LIBC_FLOATINGPOINT?
https://github.com/apache/nuttx-apps/blob/master/system/uorb/Kconfig#L43

@linguini1
Copy link
Contributor Author

Do you enable DEBUG_UORB, which auto select LIBC_FLOATINGPOINT? https://github.com/apache/nuttx-apps/blob/master/system/uorb/Kconfig#L43

Yes, that is what I'm enabling. Although it doesn't enable printing floating points, I guess LIBC_PRINT_EXTENSION is different? I would want to add select LIBC_FLOATINGPOINT to that.

@xiaoxiang781216
Copy link
Contributor

Yes, we need select LIBC_FLOATINGPOINT too.

@raiden00pl
Copy link
Member

raiden00pl commented Jan 22, 2025

@xiaoxiang781216 why we should use select here and in other places it's not recommended? Either we are good with select (which I think is a bad solution) and we can use it in other places, or we avoid it and use select only for helper Kconfig options (that are not visible to user). We should be consistent here.

@xiaoxiang781216
Copy link
Contributor

It's OK to change to dependence. I mention select just because I find many place use "select LIBC_FLOATINGPOINT".

@raiden00pl
Copy link
Member

It would be good if we stick to one version for all new changes, otherwise it gets confusing. Currently we have select in many places as you said, but adding more select will only make this situation worse. I think it would be better to slowly replace wrong select with depends on than introduce new select to the code.

@linguini1
Copy link
Contributor Author

My concern with depends is that as far as I'm aware, the user has to enable all dependencies before the option becomes visible to select. Please correct me if I'm wrong. Otherwise I think I could just add a #warning directive about lacking floating point printing.

@raiden00pl
Copy link
Member

raiden00pl commented Jan 23, 2025

My concern with depends is that as far as I'm aware, the user has to enable all dependencies before the option becomes visible to select. Please correct me if I'm wrong. Otherwise I think I could just add a #warning directive about lacking floating point printing.

Yes, that's a problem. But I see this more like Kconfig limitation, where there really is no perfect solution. In such a case we have to choose the least harmful solution, which in this case is to avoid select. This approach is common in other large Kconfig based projects like Linux or Zephyr.

If you want see all available options, I recommend to use kconfiglib with [A] Toggle show-all mode. Then you can see all available options, event those with unmet depends on.

@linguini1
Copy link
Contributor Author

But I see this more like Kconfig limitation, where there really is no perfect solution.

That makes sense. I am surprised that something as widely used as Kconfig does not have the ability to recursively select dependencies.

The "toggle all" is a good solution in the interim!

@acassis
Copy link
Contributor

acassis commented Jan 23, 2025

@raiden00pl I think kconfig-frontends also support to see all the entries, but I think it is the [Z] instead of [A]. Unfortunately these tools are diverging (extend and kill approach?), kconfiglib added features that are missing on kconfig-frontend:
open-vela/docs#52

@raiden00pl
Copy link
Member

@acassis that's funny, never saw this option in kconfig-frontend :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: all Issues that apply to all architectures Area: Configuring Configuring issues OS: Linux Issues related to Linux (building system, etc) Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants