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
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions src/main/msp/msp.c
Original file line number Diff line number Diff line change
@@ -124,6 +124,7 @@
#include "pg/usb.h"
#include "pg/vcd.h"
#include "pg/vtx_table.h"
#include "pg/sbus_output.h"

#include "rx/rx.h"
#include "rx/rx_bind.h"
@@ -1011,7 +1012,7 @@ static bool mspCommonProcessOutCommand(int16_t cmdMSP, sbuf_t *dst, mspPostProce
*
* sbufWriteU8(dst, currentPidProfile->yourFancyParameterA);
* sbufWriteU8(dst, currentPidProfile->yourFancyParameterB);
*/
*/
break;

default:
@@ -1696,6 +1697,18 @@ static bool mspProcessOutCommand(int16_t cmdMSP, sbuf_t *dst)
break;
#endif

#ifdef USE_SBUS_OUTPUT
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;
#endif

case MSP_DATAFLASH_SUMMARY:
serializeDataflashSummaryReply(dst);
break;
@@ -3238,6 +3251,28 @@ static mspResult_e mspProcessInCommand(mspDescriptor_t srcDesc, int16_t cmdMSP,
break;
#endif

#ifdef USE_SBUS_OUTPUT
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);
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)

}
}
break;
}
#endif

case MSP_SET_NAME:
memset(pilotConfigMutable()->name, 0, ARRAYLEN(pilotConfig()->name));
for (unsigned int i = 0; i < MIN(MAX_NAME_LENGTH, dataSize); i++) {
@@ -3565,7 +3600,7 @@ static mspResult_e mspCommonProcessInCommand(mspDescriptor_t srcDesc, int16_t cm
* currentPidProfile->yourFancyParameterA = sbufReadU8(src);
* currentPidProfile->yourFancyParameterB = sbufReadU8(src);
* }
*/
*/
break;

default:
2 changes: 2 additions & 0 deletions src/main/msp/msp_protocol.h
Original file line number Diff line number Diff line change
@@ -193,6 +193,8 @@
#define MSP_SET_GOVERNOR_PROFILE 149
#define MSP_LED_STRIP_SETTINGS 150
#define MSP_SET_LED_STRIP_SETTINGS 151
#define MSP_SBUS_OUTPUT_CONFIG 152
#define MSP_SET_SBUS_OUTPUT_CONFIG 153

#define MSP_EXPERIMENTAL 158
#define MSP_SET_EXPERIMENTAL 159