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

Flip over after crash aka TURTLE MODE #6685

Merged
merged 14 commits into from Mar 20, 2021

Conversation

kernel-machine
Copy link
Contributor

Since there are many users that require this feature, included me, i decide to implement it #5126

This feature is a must have for bando pilots, so maybe they will use also iNav.

The main function applyFlipOverAfterCrashModeToMotors is taken by betaflight with some edits and i have write a function to send dshot commands to the ESCs, using the same protocol used by betaflight, send the packet 10 times with the telemetry flag active with an interval 1000 uS between a packet and the next.
I have not found an official DSHOT command manual so i've based my implementation on what betaflight does.

On OSD and on iNav configurator this mode is called respectively TURT and TURTLE to avoid to use the longer name "Flip over after crash"

The power factor of 0.65 is fine for my 5", but i have not way to try this on another quads

@Sebbb
Copy link

Sebbb commented Mar 7, 2021

Hello,

great work, I tried it on 2.6.1 and it works perfect :)

Small remark: I have "set nav_extra_arming_safety = ALLOW_BYPASS" enabled, to arm if GPS signal is lost. So when I press the yaw stick to the lower right, and arm in turtle mode, it spins up like hell.

I would expect turtle mode to also work without reliable GPS signal.

@avsaase
Copy link
Member

avsaase commented Mar 7, 2021

Small remark: I have "set nav_extra_arming_safety = ALLOW_BYPASS" enabled, to arm if GPS signal is lost. So when I press the yaw stick to the lower right, and arm in turtle mode, it spins up like hell.

This also happens without this PR. I prefer arming by flipping the arm switch 10 times.

@Sebbb
Copy link

Sebbb commented Mar 7, 2021

Small remark: I have "set nav_extra_arming_safety = ALLOW_BYPASS" enabled, to arm if GPS signal is lost. So when I press the yaw stick to the lower right, and arm in turtle mode, it spins up like hell.

This also happens without this PR. I prefer arming by flipping the arm switch 10 times.

No, as in normal mode, your throttle is low when you arm, so it does not spin up differently. But here the yaw stick has a function, regardless of the throttle set.

@Sebbb
Copy link

Sebbb commented Mar 7, 2021

But indeed, I could build that with a logic condition, to disable safety checks if that mode channel is set

@avsaase
Copy link
Member

avsaase commented Mar 7, 2021

If you have airmode_type = STICK_CENTER then arming with the yaw stick to the right will immediately cause the quad to try to yaw so the motors spin up a lot. See #6531.

@Sebbb
Copy link

Sebbb commented Mar 7, 2021

If you have airmode_type = STICK_CENTER then arming with the yaw stick to the right will immediately cause the quad to try to yaw so the motors spin up a lot. See #6531.

You are right. My Quad has mc_airmode_type = THROTTLE_THRESHOLD, so I never noticed that. Interestingly, the settings documentation seems to be wrong, it's telling default value is STICK_CENTER but I never changed it.

Another thing, with disarm_kill_switch = OFF Turtle mode stays on if throttle is not low. Which is the expected behavior, but for turtle mode not intuitive, as propellers don't spin while the quad is still armed.

@kernel-machine
Copy link
Contributor Author

I would expect turtle mode to also work without reliable GPS signal.

The only safety check that i've done is the throttle level, to me it works without a GPS fix, then when Navigation is safe is not checked in configurator pre-arming checks

@Sebbb
Copy link

Sebbb commented Mar 7, 2021

The only safety check that i've done is the throttle level, to me it works without a GPS fix, then when Navigation is safe is not checked in configurator pre-arming checks

Yes, I have set this. In normal mode I don't want it to arm without a GPS fix. But anyway, I can get my expected behavior via programming (OVERRIDE ARMING SAFETY with a RC Channel condition).

Great work :)

@kernel-machine
Copy link
Contributor Author

561f13b while i was upgrade the pg version that i had forgotten to do, i realize that save the power factor on the rates profiles have not much sense, so i moved it on motorConfig

@DzikuVx DzikuVx added this to the 2.7 milestone Mar 12, 2021
Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

LGTM in general. The only request is to change the DSHOT command infrastructure a bit to be more generic so we can reuse it for DSHOT beeper in the future.

@@ -568,6 +570,24 @@ void pwmWriteServo(uint8_t index, uint16_t value)
}
}

#ifdef USE_DSHOT
void changeDshotSpinRotation(dshotDirectionCommands_e directionSpin) {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to do this differently. All motor interaction should happen inside pwm_output, there shouldn't be any external APIs calling functions that do an async motor writes.

I suggest to create a pending command structure and have an API (this function) to only update that pending command and return immediately. Actual sending data to the motors should happen inside pwmCompleteMotorUpdate.

This will allow us to reuse this code infrastructure in the future to implement DSHOT beeper, or in-flight motor direction change.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @digitalentity over here. This function should only set a flag, do not call any other functions, and logic of sending the command should happen the next time when pwmCompleteMotorUpdate is called

src/main/fc/settings.yaml Show resolved Hide resolved
@@ -568,6 +570,24 @@ void pwmWriteServo(uint8_t index, uint16_t value)
}
}

#ifdef USE_DSHOT
void changeDshotSpinRotation(dshotDirectionCommands_e directionSpin) {
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @digitalentity over here. This function should only set a flag, do not call any other functions, and logic of sending the command should happen the next time when pwmCompleteMotorUpdate is called

@DzikuVx
Copy link
Member

DzikuVx commented Mar 13, 2021

@kernel-machine besides requested changes, amazing work man! Super cool and useful addition to INAV!

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

A minor optimization to avoid queueing the same command multiple times.


//Check if there are commands to send
if(!circularBufferIsEmpty(&commandsCircularBuffer) &&
micros() - lastCommandSent > DSHOT_COMMAND_INTERVAL_US){
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the logic we want. It's safe to assume that we WANT all DSHOT commands to arrive to the ESC before continuing sending motor values. Also for robustness we should send each DSHOT command several times - 8? 16? This is something we need to define (or borrow from BF).

If we have commands pending - we should be sending only commands, no scheduling or interleaving.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should still keep the queue (in case we would want to send several different commands), but to save queue depth, it might be better to implement something like a static struct:

typedef struct {
    dshotCommands_e cmd;
    int remainingRepeats;
} currentExecutingCommand_t;

When we reach remainingRepeats == 0 we look at the queue to check if there is a new command pending. If there is a command - we pop it, update the cmd and initialize remainingRepeats to repeat count.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is the logic we want. It's safe to assume that we WANT all DSHOT commands to arrive to the ESC before continuing sending motor values. Also for robustness we should send each DSHOT command several times - 8? 16? This is something we need to define (or borrow from BF).

I've tested with different number of repetition, and it work only with a number of repetition >= 6, betaflight use 1 for BEACON commands and 10 for all others commands (included direction spin commands), so maybe 10 is the right choose

If we have commands pending - we should be sending only commands, no scheduling or interleaving.

I've just tested without the time interval between the commands and it works, i don't know why betaflight does this.

I think we should still keep the queue

Yes i agree, it was not a good idea to store a command several time on the queue

circularBufferPopHead(&commandsCircularBuffer,(uint8_t*) &cmd);

for (uint8_t i = 0; i < getMotorCount(); i++) {
pwmRequestMotorTelemetry(i);
Copy link
Member

Choose a reason for hiding this comment

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

No need to use abstractions - we know what data values we have to update so we can update motors[index].value and motors[index].requestTelemetry directly.

}

for(uint8_t i = 0; i<repeats;i++) {
circularBufferPushElement(&commandsCircularBuffer,(uint8_t*)&cmd);
Copy link
Member

Choose a reason for hiding this comment

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

Wounldn't it be easier to add a repeat counter to a DSHOT command?

@kernel-machine
Copy link
Contributor Author

kernel-machine commented Mar 18, 2021

I've edit the PR as requested by @digitalentity, i have moved the computation on a new function executeDShotCommands called by pwmCompleteMotorUpdate.

I've found this on BLHeli repository https://github.com/bitdump/BLHeli/blob/master/BLHeli_S%20SiLabs/Dshotprog%20spec%20BLHeli_S.txt
We can see that direction spin commands must be sent 6 times and is not required a delay between the commands

...
20 DSHOT_CMD_SPIN_DIRECTION_NORMAL,// Need 6x, no wait required
21 DSHOT_CMD_SPIN_DIRECTION_REVERSED,	// Need 6x, no wait required
...

So i have set the number to repetitions to 6 and removed the delay between the commands

As already said betaflight sends this commands 10 times and has a timer between the commands, i'm not understand why BF have this delay, but with 6 times and without the delay works fine with my ESCs, and for how is implemented in iNav the motor interaction i think that we are quite sure that all 6 commands are sent to the ESCs.

In the future when we implements the others dshot commands for some of them a delay between the commands must be required, but for the turtle mode this can be enough

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

If we spot room for further improvements we can do them in separate PRs

@digitalentity
Copy link
Member

Awesome work @kernel-machine thanks!

@DzikuVx DzikuVx merged commit c29f67a into iNavFlight:master Mar 20, 2021
@marianomd
Copy link
Contributor

This is very nice! Thanks @kernel-machine !!

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

6 participants