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

refactor(rc channels): Use packed RC channels #30

Merged
merged 10 commits into from
Jul 21, 2023

Conversation

ZZ-Cat
Copy link
Owner

@ZZ-Cat ZZ-Cat commented Jul 21, 2023

Overview

This Pull Request re-factors the way RC channels data is handled.

What's changed

The Old Way

In the CRSFforArduino::update() function, this is how I used to unpack RC channels data...

// Read the RC channels.
// 11 bits per channel, 16 channels = 176 bits = 22 bytes.
_channels[0] = (uint16_t)((_crsfFrame.frame.payload[0] | _crsfFrame.frame.payload[1] << 8) & 0x07FF);
_channels[1] = (uint16_t)((_crsfFrame.frame.payload[1] >> 3 | _crsfFrame.frame.payload[2] << 5) & 0x07FF);
_channels[2] = (uint16_t)((_crsfFrame.frame.payload[2] >> 6 | _crsfFrame.frame.payload[3] << 2 | _crsfFrame.frame.payload[4] << 10) & 0x07FF);
_channels[3] = (uint16_t)((_crsfFrame.frame.payload[4] >> 1 | _crsfFrame.frame.payload[5] << 7) & 0x07FF);
_channels[4] = (uint16_t)((_crsfFrame.frame.payload[5] >> 4 | _crsfFrame.frame.payload[6] << 4) & 0x07FF);
_channels[5] = (uint16_t)((_crsfFrame.frame.payload[6] >> 7 | _crsfFrame.frame.payload[7] << 1 | _crsfFrame.frame.payload[8] << 9) & 0x07FF);
_channels[6] = (uint16_t)((_crsfFrame.frame.payload[8] >> 2 | _crsfFrame.frame.payload[9] << 6) & 0x07FF);
_channels[7] = (uint16_t)((_crsfFrame.frame.payload[9] >> 5 | _crsfFrame.frame.payload[10] << 3) & 0x07FF);
_channels[8] = (uint16_t)((_crsfFrame.frame.payload[11] | _crsfFrame.frame.payload[12] << 8) & 0x07FF);
_channels[9] = (uint16_t)((_crsfFrame.frame.payload[12] >> 3 | _crsfFrame.frame.payload[13] << 5) & 0x07FF);
_channels[10] = (uint16_t)((_crsfFrame.frame.payload[13] >> 6 | _crsfFrame.frame.payload[14] << 2 | _crsfFrame.frame.payload[15] << 10) & 0x07FF);
_channels[11] = (uint16_t)((_crsfFrame.frame.payload[15] >> 1 | _crsfFrame.frame.payload[16] << 7) & 0x07FF);
_channels[12] = (uint16_t)((_crsfFrame.frame.payload[16] >> 4 | _crsfFrame.frame.payload[17] << 4) & 0x07FF);
_channels[13] = (uint16_t)((_crsfFrame.frame.payload[17] >> 7 | _crsfFrame.frame.payload[18] << 1 | _crsfFrame.frame.payload[19] << 9) & 0x07FF);
_channels[14] = (uint16_t)((_crsfFrame.frame.payload[19] >> 2 | _crsfFrame.frame.payload[20] << 6) & 0x07FF);
_channels[15] = (uint16_t)((_crsfFrame.frame.payload[20] >> 5 | _crsfFrame.frame.payload[21] << 3) & 0x07FF);

This is a very inefficient way of doing it, plus visually speaking, it looks confusing af.

The New Way

In the CRSFforArduino::update() function, this is how I roll, now...

// Read the RC channels.
memcpy(&_crsfRcChannelsPackedFrame, &_crsfFrame, CRSF_FRAME_SIZE_MAX);

Notice how it's much more concise?
Programmatically, it is more efficient because all I am doing here is double-buffering the incoming RC channel data.
I have also moved the RC channel data unpacking out of the CRSFforArduino::update() function & put it into the CRSFforArduino::getChannel() function.

const __crsf_rcChannelsPacked_t *rcChannelsPacked = (__crsf_rcChannelsPacked_t *)&_crsfRcChannelsPackedFrame.frame.payload;

// Unpack the RC channels.
_channels[RC_CHANNEL_ROLL] = rcChannelsPacked->channel0;
_channels[RC_CHANNEL_PITCH] = rcChannelsPacked->channel1;
_channels[RC_CHANNEL_THROTTLE] = rcChannelsPacked->channel2;
_channels[RC_CHANNEL_YAW] = rcChannelsPacked->channel3;
_channels[RC_CHANNEL_AUX1] = rcChannelsPacked->channel4;
_channels[RC_CHANNEL_AUX2] = rcChannelsPacked->channel5;
_channels[RC_CHANNEL_AUX3] = rcChannelsPacked->channel6;
_channels[RC_CHANNEL_AUX4] = rcChannelsPacked->channel7;
_channels[RC_CHANNEL_AUX5] = rcChannelsPacked->channel8;
_channels[RC_CHANNEL_AUX6] = rcChannelsPacked->channel9;
_channels[RC_CHANNEL_AUX7] = rcChannelsPacked->channel10;
_channels[RC_CHANNEL_AUX8] = rcChannelsPacked->channel11;
_channels[RC_CHANNEL_AUX9] = rcChannelsPacked->channel12;
_channels[RC_CHANNEL_AUX10] = rcChannelsPacked->channel13;
_channels[RC_CHANNEL_AUX11] = rcChannelsPacked->channel14;
_channels[RC_CHANNEL_AUX12] = rcChannelsPacked->channel15;

Here you can see how straightforward this looks. I know what RC channel is going where.

Additional

I have removed the caveat regarding the SERCOM UART data order.
Since using packed RC channels, there is no need for me to the low-level hardware UART driver to set its data order to Most Significant Bit First.
This opens up the door for more hardware compatibility.

In the readme I have also clarified what CRSF for Arduino's minimum requirements are, & what you can do if your devboard meets the minimum requirements & yet, you're still having compatibility issues.

@ZZ-Cat ZZ-Cat self-assigned this Jul 21, 2023
@ZZ-Cat ZZ-Cat marked this pull request as ready for review July 21, 2023 05:07
@ZZ-Cat ZZ-Cat merged commit d39bba0 into Main-Trunk Jul 21, 2023
@ZZ-Cat ZZ-Cat deleted the testing-packedRCchannels branch July 21, 2023 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant