-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fixed code of Smartnest and updated documentation #4001
Conversation
@Aircoookie & @Steven17D can you verify the changes please? This looks like a final version after much troubles. |
usermods/smartnest/readme.md
Outdated
|
||
## Change log | ||
2022-09 | ||
* First implementation. | ||
2024-05 |
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 changelog append the changes instead of removing the old log.
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.
Fixed up
lastMqttReport = currentMillis; | ||
|
||
// Report current brightness | ||
char brightnessMsg[6]; |
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.
Max %u output may be 4,294,967,295 which needs 11 characters (including \0)
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.
Fixed up
@@ -9,6 +9,10 @@ | |||
class Smartnest : public Usermod | |||
{ | |||
private: | |||
bool initialized = false; | |||
unsigned long lastMqttReport = 0; | |||
unsigned long mqttReportInterval = 60000; // Report every minute |
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.
make this static const (or constexpr is better)
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 would argue to allow it to be just unsigned
and use it as a run-time settable variable.
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.
So I better leave it like this?
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.
Better to add configuration item like other usermods.
*/ | ||
void setup() { | ||
// Initialization code here | ||
if (!initialized) { |
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.
Why is initialized
needed? This function is called once (isn't 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.
Exactly my thoughts. 😄
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.
Fixed up
usermods/smartnest/readme.md
Outdated
2. Use `#define USERMOD_SMARTNEST` in wled.h or `-D USERMOD_SMARTNEST` in your platformio.ini | ||
1. Use `#define USERMOD_SMARTNEST` in wled.h or `-D USERMOD_SMARTNEST` in your platformio.ini (recommended). | ||
## It is not necessary since the main branch of WLED brings it with 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.
If this is not necessary - remove
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.
Fixed up
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.
No new commit
Sorry! I was on the wrong project branch, I'm sorry for the inconvenience! Now look |
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.
Looks good to me. Thanks for your contribution!
All problems were already corrected and working now.