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

Adding OTA Agent with suspend resume. #37

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Adding OTA Agent with suspend resume. #37

merged 1 commit into from
Sep 26, 2023

Conversation

manvensh
Copy link
Contributor

@manvensh manvensh commented Aug 31, 2023

Adding OTA Agent with suspend resume.

Testing

  1. Created a new task which suspends and resume the OTA-agent in a loop.
  2. Tested the OTA-agent by triggering the OTA job and making sure the correct file is getting downloaded. Also, checked that the OTA-agent is getting suspended and resumed properly.

demo/os/ota_os_freertos.c Outdated Show resolved Hide resolved
demo/os/ota_os_freertos.c Outdated Show resolved Hide resolved
demo/os/ota_os_freertos.c Outdated Show resolved Hide resolved
demo/ota-agent/main.c Outdated Show resolved Hide resolved
if( status == MQTTRecvFailed )
{
printf( "ERROR: MQTT Receive failed. Closing connection.\n" );
exit( 1 );
Copy link
Member

Choose a reason for hiding this comment

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

Is this closing the connection? Seems like it would kill the demo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this can kill the demo. We followed the same approach in the chain of responsibility demo as well.
We can add retries if don't want to stop the demo in case of network failure or connection issues.


jobIdLength = coreJobs_getJobId( (char *)jobDoc->jobData, jobDoc->jobDataLength, &jobId );

/* TODO: Change length to the length of global job id */
Copy link
Member

Choose a reason for hiding this comment

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

Does this still need to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we need to revisit this part to avoid the scenario where the jobIDLength is greater than the maximum job id length.

@kstribrnAmzn
Copy link
Member

Can you include some testing details? I know you ran the demo but seeing what the output should look like (even if truncated) would be helpful

@kstribrnAmzn
Copy link
Member

A README explaining the demo would certainly be nice to have

@kstribrnAmzn
Copy link
Member

kstribrnAmzn commented Sep 5, 2023

High level consideration: A mutex around all assignments of otaAgentState would be helpful in preventing state overrides.
The suggestions mentioned here would fix this thread safety concern.

This can be done as a follow up if we are still working on this.

Discussed offline with @manvensh. This code was specifically written to ONLY ever modify the otaAgentState within the event processing loop. I didnt' notice that on my first pass review. With that in mind, there shouldn't be any thread safety issues.

@manvensh
Copy link
Contributor Author

A README explaining the demo would certainly be nice to have

Working on it and will add it as a part of next PR.

Copy link
Member

@kstribrnAmzn kstribrnAmzn left a comment

Choose a reason for hiding this comment

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

Would still like to see the demo output on the CR description

@manvensh manvensh merged commit c522009 into main Sep 26, 2023
@manvensh manvensh deleted the adding_ota_agent branch September 26, 2023 23:11
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