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

#536 ability to enable high-level commands after low-level commands #560

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

jpreiss
Copy link
Contributor

@jpreiss jpreiss commented Feb 27, 2020

Extensive discussion of the code design is in #536. This code is tested on real hardware. Confirmed that we can:

  • Take off in high-level mode
  • Fly using streaming setpoints (I tested velocity_world and full_state)
  • Re-enable high-level mode
  • Immediately land or do a goTo

I was not able to test manual override because I was not sure how. However, based on the design I am confident it will work.

Copy link
Contributor

@krichardsson krichardsson left a comment

Choose a reason for hiding this comment

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

Hi and thanks for the pull request!

It looks good to me , but I have two small comments.

  1. There are a bit too many DEBUG_PRINT for my taste. DEBUG_PRINT always outputs a message on the console log and I think we should keep logging during "normal" operation to a minimum, and only notify the user of exceptional events. One option is to define your local log macro, something like https://github.com/bitcraze/crazyflie-firmware/blob/master/src/deck/core/deck_info.c#L39-L43
  2. I prefer named constants in the decoding of the packet, to make it easier to understand what the purpose of the channel is. (I will try to add review comments for this in the code, I have not used this feature before in github :-))

br
Kristoffer

Comment on lines 107 to 132
switch (pk->channel) {
case 0:
crtpCommanderGenericDecodeSetpoint(&setpoint, pk);
commanderSetSetpoint(&setpoint, COMMANDER_PRIORITY_CRTP);
break;
case 1: {
uint8_t metaCmd = pk->data[0];
if (metaCmd < nMetaCommands && (metaCommandDecoders[metaCmd] != NULL)) {
metaCommandDecoders[metaCmd](pk->data + 1, pk->size - 1);
}
}
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Named constants makes it easier to understand what the cases are handling. Maybe something like

switch (pk->channel) {
  case SET_SETPOINT_CHANNEL:
    // ...
    break;
  case META_COMMAND_CHANNEL:
    // ...
    break;
  default:
    // Do nothing
    break;
}

and a default for completeness.

@jpreiss
Copy link
Contributor Author

jpreiss commented Feb 27, 2020

@krichardsson Thank you for the review! I added the channel enum and got rid of the DEBUG_PRINTs.

Following the style of crtp_commander_generic.c, I also added comments in crtp_commander.c with a numbered "to-do" list for adding a new command, and changed the comments from // to /* */ style.

@krichardsson krichardsson merged commit 1d0fa9c into bitcraze:master Feb 28, 2020
@krichardsson
Copy link
Contributor

Merged. Thanks!

@krichardsson krichardsson added this to the next-release milestone Feb 28, 2020
cafeciaojoe pushed a commit to cafeciaojoe/crazyflie-firmware that referenced this pull request Sep 27, 2024
cafeciaojoe pushed a commit to cafeciaojoe/crazyflie-firmware that referenced this pull request Sep 27, 2024
So that changes on param update will be reflected.

Fixes: bitcraze#560
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.

2 participants