search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.96k stars 6.67k forks source link

fuel-gauge API: unclear specification #77509

Open JordanYates opened 3 months ago

JordanYates commented 3 months ago

Describe the bug

Several parameters in the Fuel Gauge API are specified as one of two possible units.

https://github.com/zephyrproject-rtos/zephyr/blob/143b14bb4fb779437fdeb8369e42fbb81db4a5fd/include/zephyr/drivers/fuel_gauge.h#L81-L82 https://github.com/zephyrproject-rtos/zephyr/blob/143b14bb4fb779437fdeb8369e42fbb81db4a5fd/include/zephyr/drivers/fuel_gauge.h#L85-L86 https://github.com/zephyrproject-rtos/zephyr/blob/143b14bb4fb779437fdeb8369e42fbb81db4a5fd/include/zephyr/drivers/fuel_gauge.h#L93-L94

Which unit should drivers be providing the responses in?

Expected behavior

Applications should not need to guess the unit of the value they are querying, it should be unambiguous from the API.

teburd commented 3 months ago

These unit variances come from the Smart Battery Specification https://sbs-forum.org/specs/sbdat110.pdf

Typically there's a bit flag that describes what unit these are in that is separate from the value itself.

SzymonRichert commented 2 months ago

@JordanYates as the PR is merged can we close this issue?

JordanYates commented 2 months ago

@JordanYates as the PR is merged can we close this issue?

No, users of the API still have no way of knowing which value they are getting

aaronemassey commented 2 months ago

So the FUEL_GAUGE_SBS_ATRATE and FUEL_GAUGE_SBS_REMAINING_CAPACITY_ALARM seem appropriate to me because they're offering an entrance into an SBS spec chip.

I can absolutely see the argument that FUEL_GAUGE_DESIGN_CAPACITY should exist as a single unit. With adding a FUEL_GAUGE_SBS_DESIGN_CAPACITY to cover the smart battery spec wrt the FUEL_GAUGE_SBS_MODE property.

Really all the SBS properties were supposed to be split off, but that's another discussion (and more of just something I need to finish doing).

Making the change to FUEL_GAUGE_DESIGN_CAPACITY for the existing upstream drivers shouldn't be a huge change and would be straight-forward.

@teburd

Let me know if I'm missing anything here. I can go ahead and own making that change if we go forward with that.

teburd commented 2 months ago

My only opinion on this is that the SBS properties do need to be exposed as they are specified in some manner. If those properties are distinct from a generic fuel gauge property... that's ok, though it'd be good to provide those generic properties even for SBS.

teburd commented 2 months ago

@JordanYates I do not believe this is a bug, but by design, please re-apply if you truly believe this is a bug.