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 PPM support #20

Merged
merged 15 commits into from
Jan 25, 2019
Merged

Add PPM support #20

merged 15 commits into from
Jan 25, 2019

Conversation

ColinNiu
Copy link
Contributor

@ColinNiu ColinNiu commented Dec 26, 2018

Add PPM support.

Acceptable PPM channel value is 750~ 2250us but will be mapped to 1000~2000us with thresholding applied (750 ~1000us=>1000, 2000 ~2250us=>2000).

@kilrah
Copy link
Contributor

kilrah commented Dec 26, 2018

The 1000-2000 clamp is a bit short, will cause clipping with common systems.
Common flight controllers actually need the whole 1000-2000 range by default, an it's common to set a channel beyond that for error detection. OpenTX/er9x radios put out 988-2012 by default, more with extended limits enabled.
Would recommend supporting something like 850-2150 unless there's something internal preventing it.

@ColinNiu
Copy link
Contributor Author

Hi @kilrah , yes, already tried, but method rc_data_update_channel in rc/rc_data.c will crop the channel value into range RC_CHANNEL_MIN_VALUE ~ RC_CHANNEL_MAX_VALUE.

About using special ppm value to do error detection, I think it happens on the Rx end (as a failsafe indicator), so it may not be a big problem (for RC->Raven Tx)?

That's just my opinion, how do you think?

@ColinNiu
Copy link
Contributor Author

or... maybe we can limit valid ppm channel values to a more reasonable range, beyond which we say it's in the failsafe state? Then what would it be?

@kilrah
Copy link
Contributor

kilrah commented Dec 26, 2018

Best would be to figure out what scaling is applied between the CRSF protocol and PWM values, and allow as PPM in whatever's possible within RC_CHANNEL_MIN_VALUE and RC_CHANNEL_MAX_VALUE.

I doubt CRSF clips at 1000-2000 "equivalent" since IRC got so much flak before them for not having a bigger range on the EzUHF.

@ColinNiu
Copy link
Contributor Author

ColinNiu commented Dec 26, 2018

CRSF uses 172 ~ 1811, and I found here, SBUS uses the same range (without endpoint change).
If mapped to PWM, CRSF value 0 would be PWM 895, and CRSF max possible value 2047 (unsigned 11bit) would be 2144 in PWM.
CRSF |0--[172------1811]--2047| => PWM |895--[1000------2000]--2144|

To support the extra range, the RC part of Raven system may need to be revised.

@fiam
Copy link
Collaborator

fiam commented Jan 9, 2019

Fixes #13

@SJNASJNA
Copy link

Thank you for your attention to my question

Copy link
Collaborator

@fiam fiam left a comment

Choose a reason for hiding this comment

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

Looks perfect except for those two minor cosmetic changes. Thanks!

main/input/input_ppm.c Show resolved Hide resolved
time_micros_t pulse_length = 0;
time_micros_t current_pulse = 0;

if(input_ppm->pulseCountInQueue == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add explicit braces here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, braces added.

    if (input_ppm->pulseCountInQueue == 0)
    {
        return false;
    }

@fiam fiam merged commit 6bf0ea0 into RavenLRS:master Jan 25, 2019
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

4 participants