battery: Initial dedicated API

[aaronemassey]

Recreation of #44846 due to deletion of Fork. See #44846 (comment)

*Edit:

RFC: #44847

[bjarki-andreasen]

This function can return the time to empty instead of using an out param, since this time should not be able to be negative.

[bjarki-andreasen]

This function can return the time to full instead of using an out param, since this time should not be able to be negative.

[bjarki-andreasen]

This function needs a timeout param, otherwise the thread could block forever if there is an issue with the battery.

typedef int (*battery_wait_for_stable)(const struct device *dev, k_timeout timeout);

[bjarki-andreasen]

If this function simply calculates time to empty from the battery capacity, then this function should not be an API call, it should be an inline helper, and simply return the time in seconds, without the out param, since the capacity and the current rate can be read with the get_property API

static inline int (*battery_calculate_time_to_empty)(int capacity, int rate)
{
    ...
    ...
    return time_remaining;
}

[bjarki-andreasen]

Please append _t to typedefs battery_cut_off _t

[bjarki-andreasen]

Sleep commands like this should be handled by the driver through the pm_device actions, like on/off/resume/suspend, preferably using the pm_device_runtime subsystem. Is there a specific reason to have it here as an API call which the pm action does not cover? It can also be part of the dts as a boolean property to enable/disable the sleep functionality pr driver instance, when pm_device action turns it off.

[bjarki-andreasen]

This source does not exist

[aaronemassey]

? It's in this PR.

[nashif]

hi @aaronemassey will you be addressing comments and feedback raised in the original PR here? :)

[aaronemassey]

Just fyi, folks. I'm including tests and other work with this pull-request. I have this set as a draft for now for that reason. There's more to come!

[aaronemassey]

hi @aaronemassey will you be addressing comments and feedback raised in the original PR here? :)

Absolutely. I just had to recreate the pull-request as I wanted folks to know I'm still actively working on this.

[rriveramcrus]

Hello @aaronemassey,

This is a pretty exciting addition to Zephyr.

Some basic additions to the proposed framework to support charging ICs could be:

/*
 * Charging power converters can operate in the ‘charging’ mode (charging
 * and/or supplying the system), idle (converter idle), or ‘output’ mode (sending
 * power from the battery to a connected accessory).
 */
enum battery_opmode {
	BATTERY_OPMODE_IDLE = 0,
	BATTERY_OPMODE_CHARGING,
	BATTERY_OPMODE_OUTPUT,
};

/*
 * Charging systems are supplied by attached sources and can supply
 * attached sinks.
 */
enum battery_extconn_attached {
	BATTERY_NO_ATTACHMENT = 0,
	BATTERY_FIXED_SOURCE,
	BATTERY_PROG_SOURCE,
	BATTERY_FIXED_SINK,
};

enum battery_property_type {
	…
	/** Desired CC charging target in mA, 0 disables charging */
	BATTERY_DESIRED_CHARGING_CURRENT,
	/** Desired CV charging target in mV */
	BATTERY_DESIRED_CHARGING_VOLTAGE,
	/** Rising input current regulation target in mA, 0 disables regulation, -1 opens INFET */
	BATTERY_INPUT_CURRENT_REGULATION,
	/** Falling input voltage regulation target in mV, 0 disables regulation */
	BATTERY_INPUT_VOLTAGE_REGULATION
	/** Reverse mode output current limit */
	BATTERY_OUTPUT_SUPPLY_CURRENT,
	/** Reverse mode output voltage limit */
	BATTERY_OUTPUT_SUPPLY_VOLTAGE,
	/** Device operating mode */
	BATTERY_OPMODE,
	/** External connection status */
	BATTERY_EXTCONN_ATTACHED,
	…
};

struct battery_get_property {
	…
	union {
		…
		/** BATTERY_DESIRED_CHARGING_CURRENT */
		int desired_charging_current;
		/** BATTERY_DESIRED_CHARGING_VOLTAGE */
		int desired_charging_voltage;
		/** BATTERY_INPUT_CURRENT_REGULATION */
		int input_current_regulation;
		/** BATTERY_INPUT_VOLTAGE_REGULATION */
		int input_voltage_regulation;
		/** BATTERY_OUTPUT_SUPPLY_CURRENT */
		int output_supply_current;
		/** BATTERY_OUTPUT_SUPPLY_VOLTAGE */
		int output_supply_voltage;
		/** BATTERY_OPMODE */
		enum battery_opmode op_mode;
		/** BATTERY_EXTCONN_ATTACHED */
		enum battery_extconn_attached extconn_attached;
	};
};

Additionally charging ICs require changing settings at runtime, so a corresponding battery_set_property_t type would need to be added.

Finally, it may be a good idea to support private properties like the sensor subsystem does with the ’SENSOR_CHAN_PRIV_START’ member, but this is a ’nice-to-have’ feature.

Let me know if you have any thoughts or questions on this!
Ricardo

[aaronemassey]

@rriveramcrus

I think these are great ideas! Once I have coverage for the API as it stands, I'll add these with stubbed tests, let me know if you're interested in collaborating on a driver/emulator that will leverage these functions.

I think we ought to have a separate charger IC device driver now. It's better to keep the device drivers specific and modular within the context of a possible future power supply system.

[rgundi]

Better to split the PR into multiple commits for easier understanding. For e.g.
a. API definition
b. Sample driver (sbs_gauge)
c. Tests/examples of usage

[rgundi]

An example is shown only for "sbs,sbs-gauge-new-api". No example for zephyr,battery. Please provide.

[aaronemassey]

Changed zephyr,battery to zephyr,fuel-gauge to be more specific in language. Currently it's just a stub that's included by the experimental sbs-gauge compatible. I think that's okay for now.

[rgundi]

Standard commands 0x0 to 0x4 are R/W. I don’t see a sbs_cmd_reg_write function in sbs_gauge.c.

[aaronemassey]

This PR is focused on adding a fuel-gauge API while maintain feature parity with the sensor version of the sbs_gauge. But you're correct this was a mistake in the original sbs gauge driver. I'll add a todo here to add an sbs_gauge write.

[aaronemassey]

This will also be part of an API extension in a future PR to allow some generic writing as well.

[rgundi]

The Standard Commands 0x25 to 0x2A are reserved. As per the Revision 1.1 of the spec, “Reserved Command Error - 0x0002” needs to be returned if there is an attempt to read/write to these codes.

[aaronemassey]

Good catch. I'll fix this.

I think it's best to remove these commands for now and keep this driver to sbs_gauge spec specific.

[rgundi]

Agree to this.

[rgundi]

Standard Command 0x24 is not defined at all in the Revision 1.1 of the spec.

[aaronemassey]

I'll fix this too. Many of these errors were propagated from the origin sbs_gauge driver. I'll make a 2nd PR fixing that one. Thanks for pointing these out!

[rgundi]

Better to name it "battery_connect_state" and have only 2 values "connected" and "disconnected" ("not_disconnected" is double negative and should be avoided in general as human brain tends to get confused around double negatives). Since this is mostly a GPIO, not sure what "battery_disconnect_error" can be.

[rgundi]

The standard commands 0x00, 0x2F, 0x3C, 0x3D, 0x3E and 0x3F are manufacture-defined functions. Different batteries may use different commands for the same functionality. A way to assign commands to these functionalities needs to be provided.

[aaronemassey]

Thanks! Adding in future PR, for now maintaining future parity with sensor sbs_gauge.

[rgundi]

Suggest battery_property_type to have all the commands mentioned in the smart battery spec. That way a separate API would not be needed for time_at_rate, time_to_empty and time_to_full.

[rgundi]

Please include APIs to support below features. These implementations are manufacture specific.
a. Enable battery shipment mode
b. Enable EMShut mode

[rgundi]

There are some commands which are manufacture-defined and are not defined in the SBS (like Peak power, peak current etc). Different batteries may have different commands for the same functionality. A way to assign commands to these functionalities needs to be provided.

[aaronemassey]

Agreed. But consider this future PR work.

[pacodinev]

An unwanted opinion from an inexperienced nobody.
This was also the opinion of another fellow user fabiobaltier in the previous PR.

In my opinion this API should take more inspiration from the Linux's power_supply subsystem. Also power_supply is not only used for implementing batteries, but also charges, adapters, etc. The Linux's API is mature, a lot of use cases and chargers/battery/coulomb counter ICs are implemented, even SBS. It is similar to Zephyr OS sensors API. It may need some rework, like event notifications, but I think it is still good enough.

power_supply.h

[teburd]

An unwanted opinion from an inexperienced nobody. This was also the opinion of another fellow user fabiobaltier in the previous PR.

In my opinion this API should take more inspiration from the Linux's power_supply subsystem. Also power_supply is not only used for implementing batteries, but also charges, adapters, etc. The Linux's API is mature, a lot of use cases and chargers/battery/coulomb counter ICs are implemented, even SBS. It is similar to Zephyr OS sensors API. It may need some rework, like event notifications, but I think it is still good enough.

power_supply.h

This really does seem like a great match, and honestly isn't that far away from what is being proposed here from all appearances.

It's kind of a neat idea though, to make this more than a battery centric API but a power supply API with all the potential attributes that comes with.

Thanks for pointing this out!

[teburd]

Last thing I missed... the dts binding path is probably not quite right.

Also add this API to the doc/develop/api/overview.rst as experimental as of the next release.

[teburd]

Don't forget "Also add this API to the doc/develop/api/overview.rst as experimental as of the next release."

[aaronemassey]

@teburd

Don't forget "Also add this API to the doc/develop/api/overview.rst as experimental as of the next release."

Where's a good location to add this? In fuel_gauge.rst?

[teburd]

@teburd

Don't forget "Also add this API to the doc/develop/api/overview.rst as experimental as of the next release."

Where's a good location to add this? In fuel_gauge.rst?

Added to the table in the file
https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/develop/api/overview.rst

[aaronemassey]

@teburd

Don't forget "Also add this API to the doc/develop/api/overview.rst as experimental as of the next release."

Where's a good location to add this? In fuel_gauge.rst?

Added to the table in the file https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/develop/api/overview.rst

Oh. Somehow I opened this file up in read-only mode so I thought I wasn't to make changes against it. Thanks!

[pacodinev]

Disclaimer: I am very inexperienced, so you should probably not waste your time with me! :)
This is based on my opinion and I am sorry for being this long.
From what I am seeing, I might be late to the party. :(

Last time, I didn't saw the RFC, but now I understand it.

My biggest issue with that RFC (and that PR) is the rarity of separate charger IC and fuel gauge IC in the embedded world. Most boards that feature some kind of battery related IC have either fuel gauge IC, charger IC or an IC that combines them both. Example is MAX77818.
This device has to have two separate drivers, because it is both a fuel gauge and a charger. This isn't impossible (Linux's MFD), but I think it is unneeded complication, at least compared to Linux's power_supply.

Yes, I know, SBS is an example of separate charger and fuel gauge (SBS battery), but firstly rarely embedded device uses SBS, and secondly if you want to change charging parameters (at least in the case you can), (most of the time) you need to communicate the change to the SBS battery, and then the SBS battery will communicate the changes to the charger.

As a last example, the very integrated AXP202 from X-Powers.
Linux gets away with using 3 instances of power_supply driver: battery, ac-power, usb-power. (I am not counting the regulator interface and etc.)
With this RFC, you will need to use 4 instances of 3 drivers: 1 - battery, 1 - charger and 2 of something else for usb and ac power monitoring, for the same functionality. And they all will have some very similar APIs.
And in both cases, you will need to have some userspace driver for more advanced processing.

[aaronemassey]

@pacodinev

Disclaimer: I am very inexperienced, so you should probably not waste your time with me! :) This is based on my opinion and I am sorry for being this long. From what I am seeing, I might be late to the party. :(

Last time, I didn't saw the RFC, but now I understand it.

My biggest issue with that RFC (and that PR) is the rarity of separate charger IC and fuel gauge IC in the embedded world. Most boards that feature some kind of battery related IC have either fuel gauge IC, charger IC or an IC that combines them both. Example is MAX77818. This device has to have two separate drivers, because it is both a fuel gauge and a charger. This isn't impossible (Linux's MFD), but I think it is unneeded complication, at least compared to Linux's power_supply.

Yes, I know, SBS is an example of separate charger and fuel gauge (SBS battery), but firstly rarely embedded device uses SBS, and secondly if you want to change charging parameters (at least in the case you can), (most of the time) you need to communicate the change to the SBS battery, and then the SBS battery will communicate the changes to the charger.

As a last example, the very integrated AXP202 from X-Powers. Linux gets away with using 3 instances of power_supply driver: battery, ac-power, usb-power. (I am not counting the regulator interface and etc.) With this RFC, you will need to use 4 instances of 3 drivers: 1 - battery, 1 - charger and 2 of something else for usb and ac power monitoring, for the same functionality. And they all will have some very similar APIs. And in both cases, you will need to have some userspace driver for more advanced processing.

Made #52443.

Let's discuss this in that issue. I'd like to keep PR comments specific to the code in the PR. Especially because as a feature is experimental it may change drastically if we discover an architectural problem.