Skip to content

doc: fuel_gauge: upate api documentation #89301

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DBS06
Copy link
Contributor

@DBS06 DBS06 commented Apr 30, 2025

As discussed in #89000 I updated the fuel gauge API documentation.

It removes outdated description and adds missing API call fuel_gauge_get_buffer_props.

removes outdated description and adds missing api call.

Signed-off-by: Philipp Steiner <[email protected]>
@fmoessbauer
Copy link
Contributor

Please also see #89225. We really should make that example more generic (as done in MR PR). Maybe we should carve this commit out first and prepare a dedicated PR.

@DBS06
Copy link
Contributor Author

DBS06 commented Apr 30, 2025

Please also see #89225. We really should make that example more generic (as done in MR PR). Maybe we should carve this commit out first and prepare a dedicated PR.

First, what do you mean by "MR PR"?
Nevertheless, I am guessing you already looked into my PR #89000.

I think the new sample I did there is pretty generic and I am open to move the sample out of the #89000 PR including the changes from this commit and add it as a separate PR.
Furthermore, from your PR I like these changes fuel_gauge: max17048@36.

If that's the way we should go, I also suggest to merge the new fuel gauge drivers from your PR (#89225) and my PR (#89000) after this PR here and the PR with the new generic sample is merged.

What do you guys (@fmoessbauer, @teburd & @aaronemassey) think about this idea?

@fmoessbauer
Copy link
Contributor

fmoessbauer commented Apr 30, 2025

Hi @DBS06, that's a lot of changes and some of them are similar. How about the following:

  1. Merge this PR. It's documentation only and fine IMHO (note, that I'm not a maintainer)
  2. Move API fixes into dedicated PR: Let's take 9400bf8. While doing so, please also double check the correctness of the voltage API definition (it states uV, but the implementations seem to report mV). See here for details: fuel_gauge: add basic support for AXP2101 chip #89225 (comment)
  3. Make the example generic (I'll take care of that in a dedicated PR)
  4. Create MRs for the driver additions (I'll take care of the AXP2101)

@DBS06
Copy link
Contributor Author

DBS06 commented Apr 30, 2025

Hi @DBS06, that's a lot of changes and some of them are similar. How about the following:

1. Merge this PR. It's documentation only and fine IMHO (note, that I'm not a maintainer)

2. Move API fixes into dedicated PR: Let's take [9400bf8](https://github.com/zephyrproject-rtos/zephyr/commit/9400bf82071eb89617b579b942461c1fdf07d0d4). While doing so, please also double check the correctness of the voltage API definition (it states uV, but the implementations seem to report mV). See here for details: [fuel_gauge: add basic support for AXP2101 chip #89225 (comment)](https://github.com/zephyrproject-rtos/zephyr/pull/89225#discussion_r2064190115)

3. Make the example generic (I'll take care of that in a dedicated PR)

4. Create MRs for the driver additions (I'll take care of the AXP2101)

Sounds like a good plan!

regarding 2.: In which unit the voltage should be, should be discussed with the maintainers I think. I also stumbled over it while implementing the LC709203F driver. Actually imho mV makes more sense compared to µV, because afaik fuel gauges typically report their voltage in mV, but actually it doesn't matter, it just needs to be defined.

regarding 3.: Should we use the generic sample which I already implemented, or do you want to create a new one? Actually I like the idea of my sample, that it goes through every public property and shows the user which properties are readable, at least. Writing properties seems to me a little bit dangerous, because compared to e.g. SFDP Flash, every fuel gauge IC does have its own kind of interface and they are not standardized.

The only thing is, until next week I have probably no time to do something further, because I am off for holidays.

@fmoessbauer
Copy link
Contributor

regarding 2.

The output of the old example indicates, that the reported value was in mV, which would be a bug. But if all drivers up until now anyways reported in mV, then IMHO it would be better to just change the spec instead of the drivers

regarding 3

Your example is much better than mine. I'll drop mine from the AXP2101 PR.

@DBS06
Copy link
Contributor Author

DBS06 commented Apr 30, 2025

The output of the old example indicates, that the reported value was in mV, which would be a bug. But if all drivers up until now anyways reported in mV, then IMHO it would be better to just change the spec instead of the drivers

Actually they all report the voltage in µV. The bq27z746 and sbs_gauge reads it from the IC in mV and converts it to µC, "only" the max17048 reads it from the IC in µV.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants