Re: RFC: Energy Management Support in Zephyr


Marcus Shawcroft <marcus.shawcroft@...>
 

On 6 October 2016 at 11:42, Chakravarty, Souvik K
<souvik.k.chakravarty(a)intel.com> wrote:

Hi,

Good to see someone working on an energy management platform API. I
don;t have strong opinions on the organization of the proposed API,
the proposal looks sensible to me.

The restriction to microkernel seems odd, there is nothing proposed
here that looks difficult/awkward or inappropriate for nano kernel.
Are there specific reason you exclude nano ?

Below are some nit pick comments:

A common theme is the provision of _unknown_ enums values. Providing
an unknown or catchall enum is sometimes necessary because it is clear
that legitimate cases exist that are not covered by the other specific
enums. For example it would be quite reasonable for a driver to be
able to report that a battery is connected, disconnected, or it is
unknown. But, providing "unknowns" in other situations can be
undesirable for a couple of reasons.
- Consider a driver that needs to report something to the application,
but the API designer did not forsee a specific event. The driver
writer has two options a) fudge it and report "unknown", b) Extend the
API to include the missing specific event. If we provide "unknown" we
encourage behaviour a) but ideally we want to encourage behaviour b).
- Consider a driver that is asked to perform some action against an
"unknown" entity, what should it do? (In this case what does it mean
for an application to ask for the sampling interval to be set for an
"unknown" entity).
- Consider an application that receives an "unknown" event
notification. It can't do anything meaningful with such an event
unless there is some implied semantic associated with the event, in
which case unknown is not an appropriate name.

In this API, it looks to me that a number of the "unknown" enums, are
undesirable. I'm interested to understand any concrete use cases we
have in mind that warrants inclusion of such "unknown" enums.

+enum em_notifier_type {
+ EM_NOTIFY_CHARGER_DISCONNECTED,
+ EM_NOTIFY_CHARGER_CONNECTED,
+ EM_NOTIFY_BATT_LEVEL_CHANGED,
+ EM_MOTIFY_BATT_TEMP_SHUTDOWN,
+ EM_NOTIFY_BATT_TEMP_CRIT,
+ EM_NOTIFY_BATT_SHUTDOWN,
+ EM_NOTIFY_BATT_FULL,
+ EM_NOTIFY_BATT_LOW,
+ EM_NOTIFY_BATT_CRIT,
+ EM_NOTIFY_UNKNOWN

This unknown looks questionable to me. An application cannot do
anything meaningful with an "unknown energy management" event.

+/** Possible entities which can be sampled: Fuel-Gauge or Temperature. */
+enum em_sample_type {
+ EM_SAMPLE_FG,
+ EM_SAMPLE_TEMP,
+ EM_SAMPLE_UNKNOWN
+};

This one also looks questionable, it doesn;t make sense to ask a
driver to sample an unknown entity.

+/** Status: Whether Charging or Discharging. */
+enum em_charging_status {
+ EM_DISCHARGING,
+ EM_MAINTANENCE,
+ EM_CHARGING,
+ EM_UNKNOWN
This unknown would seem reasonable, it is conceivable that a platform
has no idea if it is charging or not.

What would EM_MAINTENANCE signify?


+/** Information regarding the charging status of the platform */
+struct em_charging_stat {
+ /** Charger connected status: 0 - Disconnected, 1 - Connected */
+ bool charger_status;
The use of bool here requires that a driver can detected the presence
of a charger. I doubt that is always the case in all devices that we
might be deployed on.

+bool em_batt_present(void);
Similarly, bool here suggests that every platform can answer the
question with true or false, but it seems plausible to me that not all
platforms will have the HW to answer that question.

+
+/**
+ * @brief Get the battery terminal voltage
+ *
+ * Called by the application & implemented by the platform drivers
+ *
+ * @param volt pointer to voltage of the battery in mV
+ *
+ * @return 0 if success, negative error code for error
+ */
+int32_t em_batt_voltage_get(int8_t *volt);
We should list the error codes that can be returned by these
functions. Especially interesting is the appropriate error code for
"unknown".

+int32_t em_batt_charge_thresholds_set(struct em_batt_charge_thresholds
+ thresholds);
+
* missing ?


+int32_t em_batt_temperature_thresholds_set(struct
+ em_batt_temperature_thresholds thresholds);
* missing ?


Many of the API function parameters proposed could be "const".

Cheers
/Marcus

Join devel@lists.zephyrproject.org to automatically receive all group messages.