-
Notifications
You must be signed in to change notification settings - Fork 6
Feature/several improvements #17
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
base: master
Are you sure you want to change the base?
Conversation
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 think this is comment is not necessary. Since debugln(x) is macro Serial.println(x), it will not shown while not monitor it.
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.
This is great. I was forgot to move this value into Constant.h. But the value was presented as second in int. Also the value was passed into ICRM App. change data type will need changes to the app too.
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.
Maybe it would be best to store it as int in milliseconds, because the usage is always in miliseconds. Currently it is converted every time the BLE stack sends new data. I was also thinking if it would be good to have to separate values for Websockets and BLE.
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'll move USE_WEBSOCKET and USE_BLE to Constants.h like this :
// Check is BLE connection supported
// Change to `false` if you do not wish to use
#define USE_BLE defined(ESP32) and true
// Change to `false` if you do not wish to use
#define USE_WEBSOCKET true
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.
Currently, we have #define, const bool, and static constexpr in Constants.h. The best C++ way and to have it type safe would be only using static constexpr, it is type safe and is evaluated during compile time.
I would also like to change the version from double to string, so semantic versioning can be used. What do you think?