-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implemented configurable delay for feedback messages (Downloading and Installing) #49
Implemented configurable delay for feedback messages (Downloading and Installing) #49
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.
naming variables
src/Context.h
Outdated
@@ -62,6 +62,7 @@ namespace sua { | |||
std::string caFilepath = SUA_DEFAULT_CA_FILEPATH; | |||
bool downloadMode = true; | |||
bool fallbackMode = false; | |||
int messageDelay = SUA_DEFAULT_MESSAGE_DELAY; |
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.
You should add the unit as comment - [s] (also in Defaults.cpp)
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.
The name "delay" is misleading. It means that something will happen later. But here it's more a suppression/rejection interval/duration/period.
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.
You should add the unit as comment - [s] (also in Defaults.cpp)
Fixed, both
src/FSM/States/Downloading.h
Outdated
@@ -32,6 +32,9 @@ namespace sua { | |||
void onEnter(Context& ctx) override; | |||
|
|||
FotaEvent body(Context& ctx) override; | |||
|
|||
private: | |||
long long _last_update = 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.
better rename: _time_last_update; // [s]
or CamelCase?
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
src/FSM/States/Installing.h
Outdated
@@ -32,6 +32,9 @@ namespace sua { | |||
void onEnter(Context& ctx) override; | |||
|
|||
FotaEvent body(Context& ctx) override; | |||
|
|||
private: | |||
long long _last_update = 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.
name: ref. in Downloading
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
if(now_in_seconds - _last_update > ctx.messageDelay) { | ||
send(ctx, IMqttProcessor::TOPIC_FEEDBACK, MqttMessage::Downloading); | ||
_last_update = now_in_seconds; | ||
} | ||
}); |
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.
Is it ok, that the final event could not always result to a message? Perhaps downloadProgressPercentage==100 is an important value that triggers something on server side so that at least the last event should always be sent.
Same mind for installation
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
@@ -22,6 +22,8 @@ | |||
#include "FotaEvent.h" | |||
#include "Logger.h" | |||
|
|||
#include <chrono> |
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.
also to be included in Installing.cpp?
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
src/FSM/States/Downloading.cpp
Outdated
Logger::info("Download progress: {}%", ctx.desiredState.downloadProgressPercentage); | ||
send(ctx, IMqttProcessor::TOPIC_FEEDBACK, MqttMessage::Downloading); | ||
|
||
if(now_in_seconds - _last_update > ctx.messageDelay) { |
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.
Shoud be operation >=:
now - last >= 5,
e.g. n s + 5 s = (n+5) s ==> send msg
(n+5) - n >= 5
same in Installating
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
65dd60f
to
0742ff0
Compare
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.
all findings fixed - ok
… Installing)