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

OCPP 1.6 Fix: add flag for pending firmware update #306

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

klemmpnx
Copy link
Contributor

In OCPP 1.6 , a flag that signals a pending firmware update is introduced. This fixes a bug in case the last received firmware status while a pending update is not "Downloaded" but something else (such as "Installing").

@klemmpnx klemmpnx requested a review from hikinggrass December 14, 2023 13:06
@klemmpnx klemmpnx self-assigned this Dec 14, 2023
Comment on lines +2382 to +2389
if (status == FirmwareStatusEnumType::SignatureVerified) {
this->firmware_update_is_pending = true;
} else if (status == FirmwareStatusEnumType::InstallationFailed ||
status == FirmwareStatusEnumType::DownloadFailed ||
status == FirmwareStatusEnumType::InstallVerificationFailed ||
status == FirmwareStatusEnumType::InvalidSignature) {
this->firmware_update_is_pending = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use switch with enums. With that the compiler gives warnings on missing unhandled values. Try to avoid using the default case as it deactivates this functionality with the usual -Wswitch (which is in -Wall, but not -Wswitch-enum, at least in gcc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two cases are not exhaustive (on purpose) though (so I would need a default case that just breaks to indeed avoid the warning). I added a comment in the code as explanation (of course, I could still also switch to a switch)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah, it may be inconvenient and look like boilerplate to write down all cases, but it helps for reviewing and maintenance. E.g. it seems at least the Installed case must also be handled here.

Comment on lines +2376 to +2377
EVLOG_debug << "Sending FirmwareUpdateStatusNotification with status"
<< conversions::firmware_status_enum_type_to_string(status);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a generic debug-log in this->send() already? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm which one do you mean? Could be that the websocket has some generic debug log. But for me the crucial aspect was that the logs actually reveal which (and in which order) firmware status updates (meaning the value) are sent and I don't think any existing log prints those (so I left this in case the firmware update make trouble again in the future)

Signed-off-by: Fabian Klemm <[email protected]>
@klemmpnx klemmpnx requested a review from Dominik-K December 14, 2023 16:36
Signed-off-by: Fabian Klemm <[email protected]>
Signed-off-by: Fabian Klemm <[email protected]>
@klemmpnx klemmpnx merged commit 7865283 into main Dec 18, 2023
3 checks passed
@klemmpnx klemmpnx deleted the fix/fk-firmware-pending-treatment branch December 18, 2023 08:12
AssemblyJohn pushed a commit that referenced this pull request Dec 18, 2023
* add flag for pending firmware update

Signed-off-by: Fabian Klemm <[email protected]>

* add comments

Signed-off-by: Fabian Klemm <[email protected]>

* fix comment grammar

Signed-off-by: Fabian Klemm <[email protected]>

* fix typo II

Signed-off-by: Fabian Klemm <[email protected]>

---------

Signed-off-by: Fabian Klemm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants