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

SKPutRequest not updating values and untracked RequestId on Put #617

Merged
merged 2 commits into from
Jul 23, 2022

Conversation

JONA-GA
Copy link

@JONA-GA JONA-GA commented Jul 9, 2022

Hello!!
my first PR on Github...
When a PutRequest is received by SensESP , a debuglog message " Untracked RequestID" and the value changed is not updated.
The code presented in PR is working fine for my application based on SmartSwitchcontroller.

Copy link
Collaborator

@mairas mairas left a comment

Choose a reason for hiding this comment

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

Thanks for the first contribution and sorry about the delay; I'm on a holiday sail and haven't been able to dig out my computer for quite a few days!

The PR looks great; I had just one minor request on variable naming.

@@ -274,7 +275,7 @@ void WSClient::process_received_updates() {
SKListener::take_semaphore();

const std::vector<SKListener*>& listeners = SKListener::get_listeners();

const std::vector<SKPutListener*>& Plisteners = SKPutListener::get_listeners();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: SensESP is following the Google C++ Style Guide and as such, member variables should have snake_case. So, I'd rename Plisteners to put_listeners.

Copy link
Author

Choose a reason for hiding this comment

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

Nice to have holiday sail !! of course, ok for the variable name, I am not a professional programmer, just a retired biomedical engineer. I will try to read the Google C++ Style Guide.
Not sure that it's possible to optimize the code by using one vector and one loop (less program memory), I found it more readable like that.

@mairas
Copy link
Collaborator

mairas commented Jul 23, 2022

Looks great now, thanks!

@mairas mairas merged commit ec52102 into SignalK:main Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants