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

Add graceful shutdown support #844

Merged
merged 3 commits into from
Sep 8, 2021
Merged

Add graceful shutdown support #844

merged 3 commits into from
Sep 8, 2021

Conversation

jonasdn
Copy link
Contributor

@jonasdn jonasdn commented Sep 3, 2021

Currently to shut-down the Crazyflie, the nRF51 simply cut the power to the system. This was working fine until we started implementing SD-card functionality: when shutting down the system that way we have huge chances of corrupting the SD-card filesystem. This has lead to implementation of very inefficient sd-car logging procedure to decrease the probability of corruption.

This PR suggests a better solution for the shutdown problem would be to implement a graceful shutdown in the system ...

... when the nRF51 wants to shutdown the system (ie. after a button press), it sends a message to the stm32 requesting shutdown

... in the stm32, modules that wants to be notified of shutdown are called in sequence and return when they are ready for shutdown

... when all the module have returned, the stm32 sends to the nRF51 a "ready for shutdown" message

... the nRF51 cuts the power. If no "ready to shutdown" was received after a timeout, the power is cut anyway

Closes #623

Currently to shut-down the Crazyflie, the nRF51 simply cut the power
to thae system. This was working fine until we started implementing
SD-card functionality: when shutting down the system that way we have
huge chances of corrupting the SD-card filesystem. This has lead to
implementation of very inefficient sd-car logging procedure to decrease
the probability of corruption.

A better solution for the shutdown problem would be to implement a
graceful shutdown in the system ...

  ... when the nRF51 wants to shutdown the system (ie. after a button
      press), it sends a message to the stm32 requesting shutdown

  ... in the stm32, modules that wants to be notified of shutdown are
      called in sequence and return when they are ready for shutdown

  ... when all the module have returned, the stm32 sends to the nRF51 a
      "ready for shutdown" message

  ... the nRF51 cuts the power. If no "ready to shutdown" was received
      after a timeout, the power is cut anyway

Closes #623
@jonasdn
Copy link
Contributor Author

jonasdn commented Sep 3, 2021

@whoenig @ataffanel I would like to add a commit to provide graceful shutdown for the usddeck, could you help me with that? Or point me towards a source than could?

thanks!

clarification: I do not think this PR should be merged without any subsystem using the functionality

Copy link
Member

@ataffanel ataffanel left a comment

Choose a reason for hiding this comment

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

Just a nitpick otherwise I like it!

src/hal/interface/syslink.h Outdated Show resolved Hide resolved
@jonasdn jonasdn force-pushed the jonasdn/623 branch 4 times, most recently from 78bb54a to 753aba1 Compare September 6, 2021 12:57
@whoenig
Copy link
Contributor

whoenig commented Sep 6, 2021

I agree that this should only be merged after we have at least one callback registered and tested. I would sign up for the new callback after it is confirmed that the usd card can be mounted and the config be read, namely at

. In the writer-task, you could check for a new flag, e.g., not_shutdown that is initially false at . In your callback you would switch the flag to true and wait until the task is finished. This sort of wait is the tricky part of the whole design.

@jonasdn
Copy link
Contributor Author

jonasdn commented Sep 7, 2021

@ataffanel @whoenig would something like the latest commit work?

@jonasdn jonasdn force-pushed the jonasdn/623 branch 2 times, most recently from afcf401 to 181afcc Compare September 7, 2021 05:35
@@ -92,6 +93,10 @@
#define FIXED_FREQUENCY_EVENT_ID (0xFFFF)
#define FIXED_FREQUENCY_EVENT_NAME "fixedFrequency"


/* set to true when graceful shutdown is triggered */
static volatile bool in_shutdown;
Copy link
Contributor Author

@jonasdn jonasdn Sep 7, 2021

Choose a reason for hiding this comment

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

this is implicit false ... should I make it explicit?

static volatile bool in_shutdown = false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

@jonasdn jonasdn force-pushed the jonasdn/623 branch 3 times, most recently from 0e29f47 to d0cb239 Compare September 7, 2021 07:20
uint32_t timeout = 15; /* ms */
in_shutdown = true;
vTaskResume(xHandleWriteTask);
ulTaskNotifyTake(pdTRUE, timeout / portTICK_PERIOD_MS);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be ...

ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(timeout));

...?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be cleaner.

Copy link
Contributor

@whoenig whoenig left a comment

Choose a reason for hiding this comment

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

The changes look good to me - I especially appreciate your timeout rather than an indefinite wait. However, I think the change is major enough that it should actually be tested:-) Does it work for you properly?

@jonasdn
Copy link
Contributor Author

jonasdn commented Sep 7, 2021

The changes look good to me - I especially appreciate your timeout rather than an indefinite wait. However, I think the change is major enough that it should actually be tested:-) Does it work for you properly?

I have now, and fixed some bugs together with @ataffanel

@jonasdn jonasdn merged commit dd63226 into master Sep 8, 2021
@jonasdn jonasdn deleted the jonasdn/623 branch September 27, 2021 04:59
@knmcguire knmcguire added this to the next release milestone Dec 16, 2021
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.

Add graceful shutdown support
4 participants