[ch1741] OTA'd module info sent as a payload in UpdateDone message or as an ACK to UpdateDone #1270
Conversation
…nt as UpdateDone message or as a piggy-back ACK to UpdateDone.
Also fixes #1240
|
@avtolstoy would you please add examples of what the payload looks like based on the different responses? Perhaps this isn't necessary if the data is just forwarded to the event stream as-is, but would be useful to document here. EDIT: nvm, I see they are in the original PR #1097 |
communication/src/messages.cpp
Outdated
| @@ -132,15 +132,26 @@ size_t Messages::hello(uint8_t* buf, message_id_t message_id, uint8_t flags, | |||
| return len; | |||
| } | |||
|
|
|||
| size_t Messages::update_done(uint8_t* buf, message_id_t message_id, bool confirmable) | |||
| size_t Messages::update_done(uint8_t* buf, message_id_t message_id, uint8_t* result, size_t result_len, bool confirmable) | |||
There was a problem hiding this comment.
Any reason not to make the result, result_len, data, and data_len const?
There was a problem hiding this comment.
Sure, they could be const.
communication/src/spark_protocol.cpp
Outdated
| memset(buf, 0, sizeof(buf)); | ||
| if (code != ChunkReceivedCode::BAD) { | ||
| callbacks.finish_firmware_update(file, UPDATE_FLAG_SUCCESS | UPDATE_FLAG_VALIDATE_ONLY, buf); | ||
| data_len = strnlen(buf, sizeof(buf) - 1); |
There was a problem hiding this comment.
Do we include the length of the extra payload info in buf? If not, do we just rely on the size_t of the packet to know where the payload ends, since we are not sending the null terminator sizeof(buf) - 1.
There was a problem hiding this comment.
We don't include the length of the payload and we don't include \0. The payload size is easily calculated at the receiving side when parsing the packet.
I've checked and we don't include \0 for publishes either.
system/src/system_update.cpp
Outdated
| @@ -318,7 +318,12 @@ int Spark_Finish_Firmware_Update(FileTransfer::Descriptor& file, uint32_t flags, | |||
|
|
|||
| hal_module_t mod; | |||
|
|
|||
| if (flags & 1) { // update successful | |||
| if ((flags & (UPDATE_FLAG_VALIDATE_ONLY | UPDATE_FLAG_SUCCESS)) == (UPDATE_FLAG_VALIDATE_ONLY | UPDATE_FLAG_SUCCESS)) { | |||
There was a problem hiding this comment.
Just a question about (UPDATE_FLAG_VALIDATE_ONLY | UPDATE_FLAG_SUCCESS)) == (UPDATE_FLAG_VALIDATE_ONLY | UPDATE_FLAG_SUCCESS). Can this end up not being 1 somehow, what is the purpose of this change?
There was a problem hiding this comment.
(UPDATE_FLAG_VALIDATE_ONLY | UPDATE_FLAG_SUCCESS) != 1. The first if branches us into a validate-only case when both UPDATE_FLAG_VALIDATE_ONLY and UPDATE_FLAG_SUCCESS are set. The one after is the original flags & 1 which is equivalent to flags & UPDATE_FLAG_SUCCESS.
communication/makefile
Outdated
| @@ -7,6 +7,6 @@ TARGET_TYPE = a | |||
|
|
|||
| BUILD_PATH_EXT=$(COMMUNICATION_BUILD_PATH_EXT) | |||
|
|
|||
| DEPENDENCIES = hal dynalib services wiring | |||
| DEPENDENCIES = hal dynalib services wiring system | |||
There was a problem hiding this comment.
We've been very conscious to avoid coupling communication with system. For any external notifications/behaviour, please add a callback to avoid a direct coupling.
system/inc/system_update.h
Outdated
| @@ -54,6 +54,12 @@ typedef enum { | |||
| MODULE_INFO_JSON_INCLUDE_PLATFORM_ID = 0x0001 | |||
| } module_info_json_flags_t; | |||
|
|
|||
| typedef enum { | |||
There was a problem hiding this comment.
I would move these down into the comms layer as part of the definition of the notify_update_done callback definition so this layer isn't dependent upon system.
Original PR #1097 for issue #1032 introduced
spark/device/ota_resultevent that was posted after receiving a binary OTA and before performing a reboot, which carried OTA'd module info after validity has been checked. This PR removes that separate event and instead sends OTA'd module info as a payload in UpdateDone message or as an ACK to UpdateDone as disccussed in ch1741.This PR also fixes #1240.
TODO: Update acceptance test (#1153) if possible.
Doneness:
Enhancements
[PR #1270]Removesspark/device/ota_resultevent and instead sends OTA'd module info as a payload in UpdateDone message, or as an ACK to UpdateDone.Bugfix
[PR #1270][Fixes #1240]TCP Firmware will not ACK every chunk in Fast OTA mode now.