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

Use generated sysdown() function #1447

Merged
merged 1 commit into from Oct 23, 2018
Merged

Conversation

ccollins476ad
Copy link
Contributor

NOTE: This commit requires a newt tool update (a38614ebc22959231ad9597c24467e34ed0e5068).

Upon application-triggered restart, execute each package's shutdown subprocedure. This feature is discussed here:
https://lists.apache.org/thread.html/ebd53c9b56230184650ea57e5534d5bcfbfc170b97547a1a26f100b5@%3Cdev.mynewt.apache.org%3E

int
sysdown(int reason, sysdown_complete_fn *complete_cb, void *arg)
{
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should call complete_cb() here, otherwise hal_system_reset() will not get called.

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, good catch.

assert(rc == 0);

/* Call each configured sysdown callback. */
for (i = 0; sysdown_cbs[i] != NULL; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be sizeof(sysdown_cbs) / sizeof(sysdown_cbs[0])? Would save 4 bytes of text, if sysdown_cbs[] we're not terminated by a NULL...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I null-terminated the array is to handle the case where there aren't any sysdown procedures (a zero-length array being undefined). gcc does allow zero-length arrays, but I didn't think four bytes' savings justified using an extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair. Agreed.

break;

default:
return SYS_EUNKNOWN;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be better to continue calling sysdown() routines, and proceed with the shutdown?

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 agree. It doesn't make sense for sysdown to just stop like this.

NOTE: This commit requires a newt tool update
(dfaede5fbdd0c9d798714abed77b54be2d26bc06).

Upon application-triggered restart, execute each package's shutdown
subprocedure.  This feature is discussed here:
https://lists.apache.org/thread.html/ebd53c9b56230184650ea57e5534d5bcfbfc170b97547a1a26f100b5@%3Cdev.mynewt.apache.org%3E
@ccollins476ad
Copy link
Contributor Author

Thanks, @mkiiskila. I believe I have addressed all your comments as follows:

  1. If newt doesn't support the sysdown feature, sysdown() just performs a system reset.
  2. If a sysdown subprocedure returns an unknown error code, pretend it returned SYSDOWN_COMPLETE (i.e., continue shutting down).

Also, I changed the syscfg setting name from SYSDOWN to NEWT_FEATURE_SYSDOWN. I think we can avoid some confusion in the future by clearly identifying settings that newt injected by itself.

@ccollins476ad ccollins476ad merged commit 54b7b38 into apache:master Oct 23, 2018
@ccollins476ad ccollins476ad deleted the sysdown branch October 23, 2018 17:48
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.

None yet

2 participants