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

sbus output msp support #202

Closed
wants to merge 2 commits into from

Conversation

gongtao0607
Copy link
Contributor

Read:

    case MSP_SBUS_OUTPUT_CONFIG:
        for (int i = 0; i < SBUS_OUT_CHANNELS; i++) {
            sbufWriteU8(dst, sbusOutConfigMutable()->sourceType[i]);
            sbufWriteU8(dst, sbusOutConfigMutable()->sourceIndex[i]);
            sbufWriteS16(dst, sbusOutConfigMutable()->sourceRangeLow[i]);
            sbufWriteS16(dst, sbusOutConfigMutable()->sourceRangeHigh[i]);
        }
        sbufWriteU8(dst, sbusOutConfigMutable()->frameRate);
        break;

Write:

    case MSP_SET_SBUS_OUTPUT_CONFIG: {
        // Write format is customized for the size and responsiveness.
        // The first byte will be the target output channel index (0-based).
        // The following bytes will be the type/index/low/high for that channel.
        // `frameRate` is piggyback to any (all) indices for simplicity.
        if (sbufBytesRemaining(src) >= 1) {
            uint8_t index = sbufReadU8(src);
            if (index < SBUS_OUT_CHANNELS && sbufBytesRemaining(src) >= 7) {
                sbusOutConfigMutable()->sourceType[index] = sbufReadU8(src);
                sbusOutConfigMutable()->sourceIndex[index] = sbufReadU8(src);
                sbusOutConfigMutable()->sourceRangeLow[index] = sbufReadS16(src);
                sbusOutConfigMutable()->sourceRangeHigh[index] = sbufReadS16(src);

                // You need to reset FC after updating ->frameRate.
                sbusOutConfigMutable()->frameRate = sbufReadU8(src);
            }
        }
        break;
    }

@gongtao0607 gongtao0607 marked this pull request as ready for review December 9, 2024 18:12
@gongtao0607 gongtao0607 changed the title Rf sbus out msp sbus output msp support Dec 9, 2024
sbusOutConfigMutable()->sourceRangeHigh[index] = sbufReadS16(src);

// You need to reset FC after updating ->frameRate.
sbusOutConfigMutable()->frameRate = sbufReadU8(src);
Copy link
Owner

Choose a reason for hiding this comment

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

I am not convinced that repeating the framerate parameter with every channel is the right thing to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have coded the unto deal with it... But maybe a better option is to not do msp frame rate changes.. And make that cli only?

Reality is that the default frame rate is probably perfect in 99.99% of cases.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove the frameRate from this MSP call. It's not correct to do it this way.

Copy link
Contributor Author

@gongtao0607 gongtao0607 Dec 15, 2024

Choose a reason for hiding this comment

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

I am not convinced that repeating the framerate parameter with every channel is the right thing to do.

It's just a compromise of simpleness and cleanness. I feel it's not super necessary to create a separate protocol for a single uncommon byte.

I have coded the unto deal with it... But maybe a better option is to not do msp frame rate changes.. And make that cli only?

Reality is that the default frame rate is probably perfect in 99.99% of cases.

I think we should keep all tunables available through MSP, so that when ready we can configure everything from the radio.

Remove frameRate from this MSP. Then it does not need reboot either.

This isn't solving the reboot issue. Reboot is not needed if I don't change the frameRate. It is needed when changed, regardless where (other MSP or cli).
This is because frameRate is read on init. The way to improve is to add rescheduleTask here too but this doesn't seem to follow other MSP conventions (MSP shouldn't care how the params are used)

Copy link
Owner

@rotorflight rotorflight left a comment

Choose a reason for hiding this comment

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

Remove frameRate from this MSP.
Then it does not need reboot either.

@gongtao0607 gongtao0607 deleted the RF-sbus_out_msp branch December 17, 2024 19:36
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.

3 participants