-
Notifications
You must be signed in to change notification settings - Fork 0
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
BQ27441 Implementation and other improvements #8
Conversation
bykowskiolaf
commented
Feb 3, 2025
•
edited
Loading
edited
- Added bq27441 support
- Refactored sensor and peripheral apis to add status codes and get rid of magic numbers
…odes and add battery level reading to LoraWAN packet
…data representation
…better maintainability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general code looks pretty good. Kudos for using constexpr
and explicit
!
Some general remarks according to whole code to consider and introduce.
LoRaWANHandler::LoRaWANHandler() { | ||
Sensor<buzzverse_v1_BQ27441Data>* LoRaWANHandler::battery_sensor = nullptr; | ||
|
||
LoRaWANHandler::LoRaWANHandler(Sensor<buzzverse_v1_BQ27441Data>& battery_sensor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use initializer list:
LoRaWANHandler::LoRaWANHandler(Sensor<buzzverse_v1_BQ27441Data>& battery_sensor) { | |
LoRaWANHandler::LoRaWANHandler(Sensor<buzzverse_v1_BQ27441Data>& sensor) | |
: battery_sensor(&sensor) |
@@ -7,15 +7,44 @@ constexpr size_t PERIPHERAL_NAME_SIZE = 32; | |||
|
|||
class Peripheral { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very general name - I suggest to introduce namespaces everywhere ex. namespace Buzzverse { }
But it's not crucial right now.
#include "buzzverse/bq27441.pb.h" | ||
#include "sensor.hpp" | ||
|
||
class BQ27441 : public Sensor<buzzverse_v1_BQ27441Data> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like buzzverse_v1_
maybe use namespace for that instead?
Status read_data(buzzverse_v1_BQ27441Data* data) const override; | ||
|
||
private: | ||
const device* bq27441_dev; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid keeping naked pointers. It's a general advice for whole code not only here. Try to use references if possible or some smart pointers instead.