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

OTA/YModem update result #1097

Merged
merged 2 commits into from
Nov 21, 2016
Merged

OTA/YModem update result #1097

merged 2 commits into from
Nov 21, 2016

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented Aug 21, 2016

Implements #1032

When updating the device via YModem:

When updating OTA:

  • After successfully receiving the binary, the device performs validation and before rebooting publishes an event spark/device/ota_result, which contains the following JSON object:
{"r": "ok", "u": {JSON ota module info, same format as particle serial inspect}}
  • In case of an error, the same object is published. JSON ota module info might not be present
{"r": "error", "u": {JSON ota module info, same format as particle serial inspect, might not be present}}
  • A full system module info object + ota module info don't fit into 255 byte publish message limit, so only ota module info is included.

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

  • When flashing (OTA/YModem) an invalid firmware binary (that the device ignores) it will post an event describing why the binary was not applied. [Implements #1032]

@avtolstoy avtolstoy added this to the 0.7.x milestone Aug 21, 2016
@m-mcgowan
Copy link
Contributor

We should try to write tests for this:

  • unit tests in the user/test/unit package to validate the ymodem code (we can stub most of the system functionality). focus here is on testing tiny pieces of the code.
  • integration tests that save the file transmitted via ymodem to the filesystem or RAM so we can verify the file saved matches the one sent
  • acceptance tests that use the CLI to flash a file to the device (both a valid and invalid one) and verifies the result.

- OTA: "spark/device/ota_result" event
- YModem: ACK/CANCEL + string error code (validation bitmask)

formatOtaUpdateStatusEventData(flags, result, &module, buf, sizeof(buf));

Particle.publish("spark/device/ota_result", (const char*)buf, 60, PRIVATE);
Copy link
Member

Choose a reason for hiding this comment

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

Add logs for this event ( as in PR #1169 ), dumping the buffer would be a quick way to tell in logs what the result of the OTA/YModem was.

formatOtaUpdateStatusEventData(flags, result, &module, buf, sizeof(buf));

Particle.publish("spark/device/ota_result", (const char*)buf, 60, PRIVATE);
HAL_Delay_Milliseconds(1000);
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming we need this to ensure the publish is sent before we soft reset to apply the OTA update. Can we make this more synchronous, as implemented in PR #1144 ? We can do this as a separate PR after these get merged. I'll take notes on all of these review comments to make sure we don't forget :)

bool result = true;
const module_info_t* info = module->info;

buf[64] = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but I would like to see this as buf[sizeof buf - 1] = '\0';

{
bool result = true;
AppendJson json(append, append_data);
//result &= json.write('{');
Copy link
Member

Choose a reason for hiding this comment

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

Do these {} make the output exceed 255? Because it would be nice to have a properly formatted JSON object by default.

@technobly technobly merged commit a1483b2 into develop Nov 21, 2016
@technobly technobly deleted the feature/update-status branch November 21, 2016 22:18
technobly added a commit that referenced this pull request Nov 29, 2016
Acceptance tests for PR #1097 (OTA/YModem update result)
@technobly technobly removed their assignment Nov 29, 2016
@technobly technobly modified the milestones: 0.7.x, 0.6.1 Nov 29, 2016
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.

3 participants