drivers: led_gpio: minor tweaks

[mbolivar-nordic]

A follow-up PR since I did not review #34417 in time.

Two minor tweaks and a semantics change:

• fix a whitespace nit
• use gpio_pin_configure_dt()
• turn the LED on in case the percentage is nonzero

The last change patterns this driver after behavior in the Android
lights HAL, which recommends analogous behavior when the user requests
a color change in a non-RGB LED:

Do your best here. [...] If you can only do on or off, 0 is off,
anything else is on.

https://source.android.com/reference/hal/structlight__state__t

I think this behavior makes more sense.

[pdunaj]

this was done in purpose.
It will allow to handle use this driver by modules that actually use brightness - values from 0 to 49 will be "dark" while 50 to 100 "bright". Almost uniform and this is the whole purpose of having this function :)

If we put zero here we will never get it dark as uint8_t is always >= 0. If we use > we would get it dark only at a single value.

[mbolivar-nordic]

This driver is really not applicable to modules like that.

I really think the android behavior makes more sense. Let's let @Mani-Sadhasivam decide as the API maintainer.

[mbolivar-nordic]

@pdunaj I did some more digging and the behavior I am proposing is also consistent with Linux:

Most LEDs don’t have hardware brightness support so will just be turned on for non-zero brightness settings.

https://www.kernel.org/doc/html/latest/leds/leds-class.html

[Mani-Sadhasivam]

I agree with @mbolivar. Having 50 as the par value seems unusual to me (I should've caught it in #34417), so let's fix the behaviour by turning the LED on if brightness is >0 and turn off if 0.

[pdunaj]

@mbolivar-nordic , @Mani-Sadhasivam I would not care so much about what Android does here. Since the argument is in percentile, it is a fraction and as such you can as well round it to the nearest integer. Sometimes it is better to rethink the usecase instead of blindly copy someone's else.
Consider the following - when application sends a triangle wave (or anything similar) to led device it will get nice pulse if you keep 50% cutoff against (almost) always lid in the other case. Applications would be able to have a decent behavior of the LED without redefining configuration for such case.

This said, we will be able to set the app configuration in both cases. I will not try to convince you more.
Will take my nack of when you fix the fact that at the moment the LED is always lid. As I mentioned (value >= 0) is always true for value being uint8_t.

[Mani-Sadhasivam]

@mbolivar-nordic , @Mani-Sadhasivam I would not care so much about what Android does here. Since the argument is in percentile, it is a fraction and as such you can as well round it to the nearest integer. Sometimes it is better to rethink the usecase instead of blindly copy someone's else.
Consider the following - when application sends a triangle wave (or anything similar) to led device it will get nice pulse if you keep 50% cutoff against (almost) always lid in the other case. Applications would be able to have a decent behavior of the LED without redefining configuration for such case.

Well, it is not about blindly copying. The user would know he is trying to use a GPIO for toggling an LED and in that case, the user is pretty much aware of the limitations like controlling the brightness. If you don't have any hardware controlled brightness, then it is obvious that >0 is high and 0 is low. That said, I'm not against the idea of 50% cutoff (maybe that's why I didn't get bothered in your PR), but it somewhat different than the usual conventions.

Will take my nack of when you fix the fact that at the moment the LED is always lid. As I mentioned (value >= 0) is always true for value being uint8_t.

Oh yeah, agree. @mbolivar Please fix it.

[pdunaj]

@Mani-Sadhasivam

The user would know he is trying to use a GPIO for toggling an LED and in that case, the user is pretty much aware of the limitations like controlling the brightness.

Sure is but the point is about the application compatibility. The great thing is when you can simply compile you application to HW that may have a limited functionality, but is still behaving well. The great example was with color TV - it was designed in a way that made signal work on BW TVs too (but simply w/o colors).
What I meant is if we keep rounding scheme some applications may be compatible right away without much configuration.

then it is obvious that >0 is high and 0 is low.

It kind of is when people threat is as boolean value instead of what it really is (fraction). One could argue that 99% brightness should be off as you cannot represent fraction on led that only goes on and off.

I guess the point is clear on both sides so there is no reason to go on with it. After the >= typo is fixed I will approve regardless.

[mbolivar-nordic]

It kind of is when people threat is as boolean value instead of what it really is (fraction). One could argue that 99% brightness should be off as you cannot represent fraction on led that only goes on and off.

I think if you're going to take this argument to its conclusion, we should just remove the brightness callback implementation from this driver entirely, since it's marked optional in the API docs.

If we're going to support it, though, I think compatibility with Linux and Android is different than 'blind copying'.

After the >= typo is fixed I will approve regardless.

Thanks.

[pdunaj]

compatibility with Linux and Android

Compatibility would mean something if you run the same SW hare and there without modification. It's not the case for obvious reasons.

should just remove the brightness callback implementation
This would on the other hand break the compatibility from the Zephyr's applications PoV. The whole point of having a common API is that you can use one code base and don't care how HW will handle it. If you remove this callback you will make us (and any other application that would like to work with leds) to code for every possible driver case our there. It is possible (people in 90s handled many possible sound cards on the market) but at the same time annoying and simply ugly.

Anyway let's stop here. Having it > 0 is good enough as we will still have a common point in the driver. Brightness pattern is controlled by our application configuration. I think it would be better if even the configuration could stay the same but I won't fight over it especially if you threaten to make the situation worse :)

[mbolivar-nordic]

Compatibility would mean something if you run the same SW hare and there without modification. It's not the case for obvious reasons.

That's a specious definition of compatibility; we can talk about behavioral compatibility without necessarily having API compatibility.

I think it would be better if even the configuration could stay the same but I won't fight over it especially if you threaten to make the situation worse :)

Fine :)

[mbolivar-nordic]

I've added a patch to the API docstring which makes the expected behavior clear.

[Mani-Sadhasivam]

LGTM!

[pdunaj]

@mbolivar-nordic , @Mani-Sadhasivam I would not care so much about what Android does here. Since the argument is in percentile, it is a fraction and as such you can as well round it to the nearest integer. Sometimes it is better to rethink the usecase instead of blindly copy someone's else.
Consider the following - when application sends a triangle wave (or anything similar) to led device it will get nice pulse if you keep 50% cutoff against (almost) always lid in the other case. Applications would be able to have a decent behavior of the LED without redefining configuration for such case.

This said, we will be able to set the app configuration in both cases. I will not try to convince you more.
Will take my nack of when you fix the fact that at the moment the LED is always lid. As I mentioned (value >= 0) is always true for value being uint8_t.

[Mani-Sadhasivam]

As spotted by @pdunaj, please use (value > 0)

[mbolivar-nordic]

As spotted by @pdunaj, please use (value > 0)

D'oh. Fixed

[simonguinot]

Hi @mbolivar-nordic,

For some reasons I missed #34417. So I am taking the opportunity of this PR to ask a couple of questions.

I remember from a previous PR (#21578) that we were concerned about the order of the children nodes not preserved across the builds. Are we good with that now ?

I also remember a proposal of yours: a DT_CHILD_INDEX macro allowing to bind a LED subnode label to an array index. Indeed an application may need some help to retrieve the index of a targeted LED. Are you still considering adding this helper ?

[MarekPieta]

For DT_INST_FOREACH_STATUS_OKAY there is information that order is not guaranteed:
https://docs.zephyrproject.org/apidoc/latest/group__devicetree-inst_gaeac7ed0f4a6820a6e5d7dadb6d62d6e7.html

For DT_FOREACH_CHILD the order should be preserved (at least that is suggested by the example usage in doc):
https://docs.zephyrproject.org/apidoc/latest/group__devicetree-generic-foreach_ga2f4eead8e8190110f5c0eb353e6a408f.html#ga2f4eead8e8190110f5c0eb353e6a408f

[pdunaj]

For DT_INST_FOREACH_STATUS_OKAY there is information that order is not guaranteed:
https://docs.zephyrproject.org/apidoc/latest/group__devicetree-inst_gaeac7ed0f4a6820a6e5d7dadb6d62d6e7.html

For DT_FOREACH_CHILD the order should be preserved (at least that is suggested by the example usage in doc):
https://docs.zephyrproject.org/apidoc/latest/group__devicetree-generic-foreach_ga2f4eead8e8190110f5c0eb353e6a408f.html#ga2f4eead8e8190110f5c0eb353e6a408f

I would be surprised if the order is not preserved between builds. This should be deterministic unless somebody made it explicitly not to. I would expect the order to change if you modify the DTS or add some overlay. But from the application perspective it is good enough as application controls what is build and when (unless somebody thinks better and tries to add soma automagically added stuff under our feet).

[mbolivar-nordic]

I also remember a proposal of yours: a DT_CHILD_INDEX macro allowing to bind a LED subnode label to an array index. Indeed an application may need some help to retrieve the index of a targeted LED. Are you still considering adding this helper ?

@simonguinot the new driver got implemented and merged before I had time to review it, so with this PR I'm just trying to get the main blocker comments I would have made addressed. With more time I would have preferred to do it the way we had discussed, using DT_CHILD_INDEX, but the 2.6 feature freeze is next week and I wasn't planning on picking up this work for this release since it had gone dormant. Sorry about that. We can revisit for LTS.

[mbolivar-nordic]

For DT_FOREACH_CHILD the order should be preserved (at least that is suggested by the example usage in doc):

I confirm that:

• DT_FOREACH_CHILD iterates over children in the same order as they appear in zephyr.dts, or it's a bug
• If that order is not reproducible across identical pristine builds, it's a bug

[simonguinot]

I also remember a proposal of yours: a DT_CHILD_INDEX macro allowing to bind a LED subnode label to an array index. Indeed an application may need some help to retrieve the index of a targeted LED. Are you still considering adding this helper ?

@simonguinot the new driver got implemented and merged before I had time to review it, so with this PR I'm just trying to get the main blocker comments I would have made addressed. With more time I would have preferred to do it the way we had discussed, using DT_CHILD_INDEX, but the 2.6 feature freeze is next week and I wasn't planning on picking up this work for this release since it had gone dormant. Sorry about that. We can revisit for LTS.

@mbolivar-nordic That's OK. The driver looks good and simple. As I see DT_CHILD_INDEX is more for the application side to convert a LED node name into an index.