Skip to content
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

drivers/at86rf215: implement battery monitor, add power bus #14973

Merged
merged 4 commits into from
Dec 1, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Sep 8, 2020

Contribution description

The at86rf215 can generate an interrupt if the supply voltage falls below a certain level.

Since such an event can have multiple consumers, a new system power bus is introduced.
Threads interested in power events can subscribe to it and will all be notified in case of an event.

New bus types for different event categories can be added in the future.

Testing procedure

Run tests/driver_at86rf215 on a board with the radio connected.

You can configure a battery monitor threshold (in mV) with

2020-09-08 11:04:59,111 #  batmon 2500

If you configure a threshold that is above the supply voltage, a battery monitor event should be triggered immediatly.

2020-09-08 11:06:11,733 #  batmon 3500
2020-09-08 11:06:11,737 # NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON

Issues/PRs references

split off #12128

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Sep 8, 2020
@kaspar030
Copy link
Contributor

I don't like msg_bus for these much. IIUC, under the hood, they msg_try_send each bus message. And then return the number of successful deliveries.

If delivery is of any importance, the sender would need to actually check this number, and compare it to the number of subscribers, (which there's no API that gets it), then handle the result somehow.

This is a recipe for disaster (unreliable software), as messages will just get lost, should the receiver msg queue be full (or not available and the receiver not waiting).

"Yeah man this is just for the battery low led" -> well...

This is a fundamental issue with asynchronous messages and statically allocated message queues, as the messages are receiver allocated, but the events are created at the sender side and might, depending on what else is going on in the system, pile up.

There are better (but less intuitive) alternatives.

IMO catching the events is important, but passing them via msg_bus is not ideal.

For lack of (working) alternative code, I'm ok to go forward with this, if we add big warnings on the messages-are-not-guaranteed-to-arrive issue.

@maribu
Copy link
Member

maribu commented Sep 8, 2020

NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON

:-'D

@benpicco benpicco force-pushed the driver/at86rf215-batmon branch from 99c9353 to a9e03fa Compare September 30, 2020 10:48
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2020
@jeandudey
Copy link
Contributor

I wonder if this sys/bus module could be used on cc26xx_cc13xx which does have a batmon and/or eventually have a common API for battery monitoring, e.g.: setting minimum voltage level, temperature if supported, etc. Or if it would be better to use SAUL for these types of reading 👀

@benpicco
Copy link
Contributor Author

benpicco commented Oct 2, 2020

Good question, the 'power bus' provides a mechanism to get notified about such evens, but no means to configure them.
I tried to come up with a more elegant way of getting a handle to the radio in #15120, but @jia200x suggested to rather use custom NETOPs.

But this could be hidden behind a generic API where the user does not need to care what provides the battery monitor.

SAUL could indeed be a good fit here. Afaik it has no concept of generating events yet, but #14182 was set to address that.

@fjmolinas
Copy link
Contributor

Can you rebase @benpicco ?

@benpicco benpicco force-pushed the driver/at86rf215-batmon branch from a9e03fa to ffc3101 Compare November 12, 2020 22:17
@benpicco
Copy link
Contributor Author

Rebased & changed it to use NETOPT as suggested in #15120 (comment)

@benpicco benpicco force-pushed the driver/at86rf215-batmon branch from ffc3101 to dbdd9d7 Compare November 13, 2020 21:59
@fjmolinas
Copy link
Contributor

Rebased & changed it to use NETOPT as suggested in #15120 (comment)

Doesn't this bloat even more the NETOPT concept?

@fjmolinas
Copy link
Contributor

@benpicco can you provide some fresh test output?

@fjmolinas
Copy link
Contributor

Rebased & changed it to use NETOPT as suggested in #15120 (comment)

Doesn't this bloat even more the NETOPT concept?

I understand in the mentioned comment it is mentioned that NETOPT is already bloated, but that is not necessarily a reason to worsen the situation. I would like one of the area maintainers to give that part an ACK (Who would that be? would it be @miri64 @jia200x ?)

@benpicco
Copy link
Contributor Author

@benpicco can you provide some fresh test output?

2020-11-16 08:46:43,070 #  batmon 2000
2020-11-16 08:46:46,510 #  batmon 3000
2020-11-16 08:46:52,822 #  batmon 3400
2020-11-16 08:46:52,826 # NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON
2020-11-16 08:46:58,214 #  batmon 3200
2020-11-16 08:47:01,182 #  batmon 3300
2020-11-16 08:47:01,186 # NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON

@fjmolinas
Copy link
Contributor

@benpicco can you provide some fresh test output?

2020-11-16 08:46:43,070 #  batmon 2000
2020-11-16 08:46:46,510 #  batmon 3000
2020-11-16 08:46:52,822 #  batmon 3400
2020-11-16 08:46:52,826 # NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON
2020-11-16 08:46:58,214 #  batmon 3200
2020-11-16 08:47:01,182 #  batmon 3300
2020-11-16 08:47:01,186 # NA NA NA NA NA NA NA NA NA NA NA NA NA BATMON

Thanks, this one is good to go IMO once the NETOPT things get's OK'ed

@benpicco
Copy link
Contributor Author

I can guard the NETOPS for the 802.15.4 modes / LoRa etc with #ifdefs as a follow up.
That should save a good amount of space already.

I think the batmon netopt makes sense since a couple of radios have it.

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, there have been no complaints on the NETOPT usage, and it was suggested by @jia200x who is quite active as a network maintainer, so I'll take that as the usage being OK.

I can guard the NETOPS for the 802.15.4 modes / LoRa etc with #ifdefs as a follow up.
That should save a good amount of space already.

+1 for this as a follow up.

@benpicco benpicco merged commit 333fce4 into RIOT-OS:master Dec 1, 2020
@benpicco benpicco deleted the driver/at86rf215-batmon branch December 1, 2020 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants