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

clear self-test flag #153

Merged
merged 4 commits into from
Feb 9, 2021
Merged

clear self-test flag #153

merged 4 commits into from
Feb 9, 2021

Conversation

pvyawaha
Copy link
Contributor

@pvyawaha pvyawaha commented Feb 4, 2021

Issue #, if available:

Description of changes:
The change clears the self-test flag.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

divekarshubham
divekarshubham previously approved these changes Feb 8, 2021
Copy link
Contributor

@gkwicker gkwicker left a comment

Choose a reason for hiding this comment

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

Can we simplify the "in self test" logic? Right now there is no single place where this information (am I in self-test?) is stored, and different parts of the code test this condition in different ways. This type of pattern introduces maintainability risk.

@@ -774,6 +774,12 @@ static OtaErr_t inSelfTestHandler( const OtaEventData_t * pEventData )
{
/* Callback for application specific self-test. */
otaAgent.OtaAppCallback( OtaJobEventStartTest, NULL );

/* Clear self-test flag. */
otaAgent.fileContext.isInSelfTest = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic determining whether or not OTA is in self-test is difficult to follow. Sometimes we call an internal function inSelfTest() which in turn calls a PAL function to see if it is in the "pending commit" state. In other cases, we examine this isInSelfTest internal member of the file context.

It also looks like the isInSelfTest member of the file context is never set to true in the code, so I'm assuming this value can only come from the service?

Overall comment here – can the logic around the "Self Test" state be simplified? As it is written it is likely to be difficult to maintain since the state information for "in self test" is located and modified in different places.

Copy link
Contributor Author

@pvyawaha pvyawaha Feb 8, 2021

Choose a reason for hiding this comment

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

This is good point. The main reason for confusion is the inSelfTest function is actually checking the platform is in self test or not. Wheras the flag in the OTA context is the runtime self-test. The inSelfTest function should be renamed to indicate that it is checking platform state platformInSelfTest. This is fixed in 17664db

@pvyawaha pvyawaha merged commit 9551f0f into main Feb 9, 2021
@pvyawaha pvyawaha deleted the clear_self_test branch February 9, 2021 18:32
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