Skip to content

Disable reboot limit #9545

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

Closed
wants to merge 1 commit into from

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Jan 30, 2019

Description

Partially revert commit cc5969b from PR #9296, which started enforcing reboot limits by correcting the comparison against zero. Now that it's working, the reboot limit is causing a problem because once it's hit the board will repeatedly halt before entering main until something corrupts or clears the error context.

This is a minimal workaround to restore the broken behaviour in 5.11.2, so that the reboot limit is again not enforced.

An alternative attempt to fix the problem properly is in PR #9544.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SenRamakri, @TeemuKultala

Partially revert commit cc5969b, which started enforcing reboot limits
by correcting the comparison against zero.

Now that it's working, the reboot limit is causing a problem because
once it's hit the board will repeatedly halt before entering main until
something corrupts or clears the error context.

This is a minimal workaround to restore the broken behaviour in 5.11.2,
so that the reboot limit is again not enforced.
@kjbracey
Copy link
Contributor Author

If we think #9544 is a working fix to get 5.11.3 working, this can be closed.

Otherwise this can be taken to master and 5.11.3 as a quick workaround, then work on #9544 as a follow-up fix for 5.11.4 or 5.12. A reversion of this would need to be added to #9544.

@cmonr
Copy link
Contributor

cmonr commented Jan 30, 2019

@SenRamakri @deepikabhavnani Please review.

As a note, only #9544 or #9545 should come in.

Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

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

Solution in #9544 seems reasonable

@@ -216,7 +216,7 @@ mbed_error_status_t mbed_error_initialize(void)
mbed_error_reboot_callback(report_error_ctx);

//We let the callback reset the error info, so check if its still valid and do the rest only if its still valid.
if (report_error_ctx->error_reboot_count > 0) {
if (report_error_ctx->error_reboot_count < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this essentially removes the bug fix we originally did. I think we should not commit this and focus on #9544.

Copy link
Contributor

@SenRamakri SenRamakri left a comment

Choose a reason for hiding this comment

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

I think this essentially removes the bug fix we originally did. I think we should not commit this and focus on #9544.

@cmonr
Copy link
Contributor

cmonr commented Jan 30, 2019

Picking #9544 over this PR.

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.

6 participants