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

RC Failsafe fix for RFD 868+/900 Modems #13218

Merged
merged 2 commits into from
Feb 20, 2020
Merged

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Oct 17, 2019

RC Failsafe isn't triggered when PPM signal is valid. These changes
update Failsafe trigger logic for RC input. RF Modules keep sending
valid PPM signal when the RC Transmitter is lost.
RF Modules which are in the air send PPM signal with same type when lost
the signal from ground.

Replacement for #12595.

Fixes #12381.

Copy link
Contributor

@bozkurthan bozkurthan left a comment

Choose a reason for hiding this comment

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

These changes should be test by other contributors.

@@ -228,6 +228,23 @@ RCUpdate::rc_poll(const ParameterHandles &parameter_handles)
/* signal is lost or no enough channels */
signal_lost = true;

//This problem occurs at RC controllers which are use PPM signal on RFD 868+/900 Modems (Maybe it can be also related another types)

} else if (rc_input.channel_count ==
Copy link
Member

Choose a reason for hiding this comment

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

You could limit this to PPM as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it PPM?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't personally used it, but that's my understanding.

image

Doing this directly in the PPM decode might be better.

https://github.com/PX4/Firmware/blob/2fcddd9b8d36b636e84f7e6c57f13904af79e430/platforms/nuttx/src/px4/stm/stm32_common/hrt/hrt.c#L447-L595

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, it would be better doing that in the PPM decoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the change seems quite specific to fix exactly one corner case, therefore I think it's worth it.

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 what you mean. It's a corner case that applies to one specific PPM receiver.

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 but it's good for that one corner case 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My take is that this is good for this corner case, and it doesn't cause any harm so it's worthwhile to be merged.

@julianoes
Copy link
Contributor Author

@bkueng any objections to get this in?

@bkueng
Copy link
Member

bkueng commented Nov 7, 2019

Actually yes, I don't like this special case that is protocol-specific in the generic code path for RC.
The least required for me is make it conditional on PPM.

@FlavioTonelli
Copy link
Contributor

I can't get why we need to add a new logic to identify such a fail. The RC_FAILS_THR and RC_MAP_FAILSAFE parameters shall be sufficient to cover that case. Am I wrong?

@julianoes
Copy link
Contributor Author

@FlavioTonelli if you have it configured correctly, yes. If not, then this can save you.

@stale
Copy link

stale bot commented Feb 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Feb 16, 2020
bozkurthan and others added 2 commits February 19, 2020 18:15
RC Failsafe isn't triggered when PPM signal is valid. These changes
update Failsafe trigger logic for RC input. RF Modules keep sending
valid PPM signal when the RC Transmitter is lost.
RF Modules which are in the air send PPM signal with same type when lost
the signal from ground.
This makes the check more specific, so it's only active on PPM input.
@julianoes julianoes force-pushed the bozkurthan-rc-failsafe-fix branch from d009461 to 7f8814e Compare February 19, 2020 17:27
@stale stale bot removed the stale label Feb 19, 2020
@julianoes
Copy link
Contributor Author

@bkueng rebased and made specific to PPM, please review again.

@bkueng bkueng merged commit e7c655b into master Feb 20, 2020
@bkueng bkueng deleted the bozkurthan-rc-failsafe-fix branch February 20, 2020 08:02
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.

Critical RC Loss Failsafe Bug, Potenial Vehicle Lost
5 participants