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

[ch1741] OTA'd module info sent as a payload in UpdateDone message or as an ACK to UpdateDone #1270

Merged
merged 5 commits into from May 9, 2017

Conversation

@avtolstoy
Copy link
Member

commented Mar 6, 2017

Original PR #1097 for issue #1032 introduced spark/device/ota_result event 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:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Enhancements

  • [PR #1270] Removes spark/device/ota_result event and instead sends OTA'd module info as a payload in UpdateDone message, or as an ACK to UpdateDone.

Bugfix

Removes spark/device/ota_result event. OTA'd module description is se…
…nt as UpdateDone message or as a piggy-back ACK to UpdateDone.

Also fixes #1240

@avtolstoy avtolstoy added this to the 0.7.0 milestone Mar 6, 2017

@technobly

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

@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

@avtolstoy avtolstoy removed the bug label Apr 6, 2017

@avtolstoy avtolstoy added the bug label May 4, 2017

@technobly technobly self-requested a review May 4, 2017

@@ -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)

This comment has been minimized.

Copy link
@technobly

technobly May 5, 2017

Member

Any reason not to make the result, result_len, data, and data_len const?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 8, 2017

Author Member

Sure, they could be const.

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);

This comment has been minimized.

Copy link
@technobly

technobly May 5, 2017

Member

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.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 8, 2017

Author Member

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.

@@ -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)) {

This comment has been minimized.

Copy link
@technobly

technobly May 5, 2017

Member

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?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 8, 2017

Author Member

(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.

@@ -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

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan May 8, 2017

Contributor

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.

@@ -54,6 +54,12 @@ typedef enum {
MODULE_INFO_JSON_INCLUDE_PLATFORM_ID = 0x0001
} module_info_json_flags_t;

typedef enum {

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan May 8, 2017

Contributor

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.

@technobly technobly merged commit 2422547 into develop May 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/update_done branch May 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.