A little bit more flexible support for battery level indication

[altishchenko]

This pull request is an extension of the PR #559, with the following changes:

1. os_getBattLevel() is kept in the code.
2. User installable callback is provided for configurations with LMIC_ENABLE_user_events setting
3. Plain setter function is provided for static or simple applications.
   Callback takes precedence over static method.

[nielskoot]

Great that you you added the callback. Just one remark, shouldn't os_getBattLevel() by default return LMIC.batteryLevel instead of MCMD_DEVS_BATT_NOINFO? That was also in the code snippet you initially proposed (#501 (comment)). I think that now LMIC_setBattLevel() and LMIC_MCMD_DEVS_BATT_DEFAULT are ignored.

[altishchenko]

Great that you you added the callback. Just one remark, shouldn't os_getBattLevel() by default return LMIC.batteryLevel instead of MCMD_DEVS_BATT_NOINFO?

Sorry, I actually added a note to PR commit diff saying so. As I said, I use callbacks and didn't notice this typo till the last moment. I had to copypaste the code from my working project.

[altishchenko]

This line should state return LMIC.batteryLevel;
it is a typo.

[cyberman54]

if you have LMIC_ENABLE_user_events enabled

The default in lmic is to support user events. So the above text maybe better should read "if you don not have disabaled ..."

[altishchenko]

Quite possibly. I am using this library in a custom-built platform specific setup, so I am not that aware of the standard defaults :)

[cyberman54]

@altishchenko I tested the LMIC_setBattery() option by merging your PR #560. It works, but i discovered, that the very first transmitted battery value is always 0xFF.

Peeking into the lmic.c shows, that the battery value is set to default (0xFF) in LMIC_reset(). My app is calling LMIC_reset() in certain states, e.g. during join sequence. So i need to change my app design.

[nielskoot]

It is an option to move the Battery Level to the client data (like clockError), that is preserved in an LMIC_reset(). I understand that LMIC_reset() is also called when there is a frame-count rollover, having the battery value reset to the default then is also not optimal.

The Battery Level initialization needs to be moved from LMIC_reset() then, but I don't understand LMIC well enough yet to where that should be done then, LMIC_init()?

[cyberman54]

@nielskoot i think the straight forward way is to use the callback option. Then we don't need an initialization, LMIC grabs the value from the user's routine just in time it needs it.

[altishchenko]

@nielskoot , @cyberman54 : I agree with both of you surely. Moving batteryLevel field to client data is a very good option if you don't use callbacks. This way battery level won't get reset upon rejoin or for other outside reasons. I will provide a patch for that today.

[altishchenko]

@nielskoot , @cyberman54 I had a closer look at the problem and there is a small problem there, it won't be initialized to the defined default, because client data is never initialized by the library, hence the default value will be 'on external power'. I am not sure if Terry (@terrillmoore ) will approve of adding such an extra complexity as client data partial initialization, it doesn't sound right to me too. Probably, this case should be considered a 'feature' and just documented.
At the moment it looks reasonable, that you should set the battery level periodically. In the real world scenario, if your protocol stack reset itself what you will get is a number (and most naturally just one or none at all) of devStatusAns messages with 'battery don't know' until your application code updates it again as a part of a routine procedure with the freshly measured battery level. If you are not using callbacks, your code is in 99.9% of cases a closed loop, where reading the battery and setting its level is one of the perpetual actions done nearly on every iteration.
Honestly, complexity involved in value preservation here does not benefit the net result it will achieve on the long run. (You will even get a bonus! You will know that your stack reset itself if your battery measure suddenly drops to unknown.)
I will wait for the Terry's word on it. I think the best way is to document the behavior.

[terrillmoore]

The current behavior (sending 0xFF if we don't have better data) was contributed by Matthijs Kooijman. I wish we could avoid adding more conditional compiles. I hate to break backward compatibility, too.

Would it not be better to just change this code to add a new API, LMIC_setBattLevel() to cache the current value in the LMIC object, and then change the MAC to use that value by default (unless overridden)? I suspect it's not really difficult for code to update the battery level before each transmit; if the value is a little stale, it's not such a big deal. (MCCI's code always transmits battery level in-band, so that it's for sure available to the app; not many networks support DevStatusReq, or make the data available to applications.

[nielskoot]

Hi Terry, being able to just set the battery level was the reason to submit my original proposal (this one is an extension), just LMIC_setBattLevel() works for me.

Some background:
The support for battery level in DevStatusAns was requested by a network. For end user support they want to have an idea and history of the device battery level, they use regular DevStatusReq for that. In-band battery level doesn't really work for them as they don't always have the keys to decode and/or know the payload format.

Thanks for the great work on LMiC

[terrillmoore]

Thanks for the kind words. Let's put this on the list for v3.3, it's an easy (and safe) change. If I understand, we must:

• add LMIC_setBatteryLevel() (and LMIC_getBatteryLevel()) in header file (note slightly different name to os_getBattLevel()
• add a private LMIC field to hold the recorded value
• change LMIC initializations to initialize the default value of the private field to 0xFF (the value that indicates that the battery level is not known). Client is required to call LMIC_setBatteryLevel() after a re-initialization. I'm avoiding the question of "what happens at LMIC_reset()", because it's used internally as well as externally -- sorting that out requires some analysis.
• change os_getBattLevel() so that it returns the private field. (The change to LMIC initialization ensures that behavior is unchanged for existing applications.)
• update the documentation

Anything else? Footprint impact of this would be minimal -- a byte of RAM and a few bytes of code for init and the change to os_getBattLevel().

[altishchenko]

So be it then. Callbacks are handier for some types of applications. I use os_getBattLevel, because, unfortunately, the tier separation within the library is very approximate and that function was just floating on the surface. I'll kill my branch then, cause I mistakenly pulled from master which is a bad thing. (But fixing that annoyance with gcc style 0b000 to the standard 0x would be great and welcome.)

[terrillmoore]

@altishchenko I will create an issue to track this feature.

BTW: yikes, I missed the 0bxxx in DevStatusReq, that crept in via a pull request, and without compiler diversity in Travis,I didn't even notice. I'll file a bug on that.