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

MSP2 over crsf done like betaflight #9528

Closed
wants to merge 1 commit into from

Conversation

druckgott
Copy link
Contributor

@druckgott druckgott commented Dec 2, 2023

I have implemented MSP2 over CRSF for the lua Script (so we get a lua script for Inav also.
betaflight/betaflight-tx-lua-scripts#491

MSP2 over crsf done like here:
betaflight/betaflight#11112
betaflight/betaflight#11131

I now get WAIT API in the Lua script which I have already don and https://github.com/klutvott123 have already updated with MSP2 over CRSF.

Maybe somebody can help me to fix the thing we get it working.
Compiling is working without error.
Currently I do not know how to debug it.

Copy link
Contributor Author

@druckgott druckgott left a comment

Choose a reason for hiding this comment

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

Please review there is a Bug But i cant find it to reciver the Information on the transmitter

@druckgott
Copy link
Contributor Author

@DzikuVx, is there a way to get a Compiler online version?

@DzikuVx
Copy link
Member

DzikuVx commented Dec 9, 2023

You mean download hex files build online? In Checks file there is an option to download all the built artifacts with all the files

@druckgott druckgott force-pushed the msp2_crfs branch 11 times, most recently from f06ecf5 to c6924ef Compare December 21, 2023 16:36
@DzikuVx
Copy link
Member

DzikuVx commented Dec 26, 2023

@druckgott tests are failing, I want to merge it to master for the next major release

@druckgott
Copy link
Contributor Author

@DzikuVx, i know i dont know whata wrong the msp 2 Do Not realy work thats why i implent testing, But it also Do Not work and i cant find why its Not working

@druckgott druckgott force-pushed the msp2_crfs branch 2 times, most recently from ad1997b to db6b8f0 Compare December 27, 2023 08:00
@druckgott druckgott closed this Mar 24, 2024
@sensei-hacker
Copy link
Collaborator

Do I understand correctly that this would make the BF Lua script also work for INAV?

@druckgott
Copy link
Contributor Author

druckgott commented Mar 24, 2024

Yes i have generate a version which should work some body from betaflight supportrd the Main point that the Script works with msp2

But i cant get it working up to now and i did Not know how to fix it

@b14ckyy
Copy link
Collaborator

b14ckyy commented Mar 24, 2024

@druckgott I suggest to keep that open. Hit me up on Discord or Facebook please I need to get you in touch with someone :D

@druckgott druckgott reopened this Mar 24, 2024
test

Update crsf.c

Update telemetry_crsf_msp_unittest.cc

Update telemetry_crsf_msp_unittest.cc

MSP2 over crsf

Update telemetry_crsf_msp_unittest.cc

MSP2 over crsf

MSP2 over crsf done like here:
betaflight/betaflight#11112
betaflight/betaflight#11131
@JimB40
Copy link

JimB40 commented Mar 27, 2024

Hey @druckgott,

Did some tests lastly on iNav MSP over CRSF and there are few problems with current implementation

@JimB40
Copy link

JimB40 commented Mar 27, 2024

  1. Missing proper MSP version in response's CRSF status byte .

When Master (radio) sends MSP packet with version 1, Slave (FC) should respond with same version number (ie 1) and properly structured MSP frame.

MSP_over_CRSF_2a

iNav sends back status byte with version always set to zero.
Checked code and version is not packed into status byte as in BetaFlight
https://github.com/betaflight/betaflight/blob/90841282b03339c7361db306d280f4bdd96de1e4/src/main/telemetry/msp_shared.c#L284-L285

Bit packing structure for status byte is:
bit 0-3 - MSP sequence ID (0-15)
bit 4 - MSP start flag (set to 1 for first CRSF frame if MSP packet is chunked over few CRSF frames, if not first chunk it is set to 0)
bit 5-6 - MSP version (1 or 2)
bit 7 - MSP error (set to 1 in case of error - '!' in MSP header)

@JimB40
Copy link

JimB40 commented Mar 28, 2024

  1. Missing MSP command in response.

MSP_over_CRSF_2b

Just after payload size (ie 3rd byte in response) there should be MSP command ID before actual payload.
Again It is missing comparing to BF
https://github.com/betaflight/betaflight/blob/90841282b03339c7361db306d280f4bdd96de1e4/src/main/telemetry/msp_shared.c#L299

and official MSP doc
MSP_over_CRSF_2b2

@JimB40
Copy link

JimB40 commented Mar 28, 2024

  1. Padding CRSF Frame containing MSP response shorter than CRSF frame is not needed.

It only slows down processing with non relevant information.
MSP_over_CRSF_3

@JimB40
Copy link

JimB40 commented Mar 28, 2024

  1. MSP checksum is optional.

CRSF frame is protected by it's own CRC, so no need to add MSP checksum.
EdgeTX is checking only CRSF frame checksum (CRC)
Haven't checked BF LUA if they check MSP CRC after decoding, but IMHO second CRC is redundant.

@JimB40
Copy link

JimB40 commented Mar 28, 2024

Basically iNav MSP even in ver 1 is missing changes that was done to BF two years ago :)
But you mentioned BF PR in first post.

To test v2 from your branch with radio running LUA app I'd need hex for KAKUTEF4V2

@b14ckyy
Copy link
Collaborator

b14ckyy commented Mar 28, 2024

I have sent you a build with this PR in it.

@sensei-hacker
Copy link
Collaborator

sensei-hacker commented Apr 3, 2024

The wrong version number and missing command ID sound like they need to be fixed, to me.
The checksum could potentially stay. After the CRSF frame is delivered to the radio, there is no guarantee about where the MSP will eventually be sent. The first use case is the Lua on the radio, but that's just the first use case.

An issue with fixing any problems present in msp_shared, outside of the CRSF context, is compatibility with legacy third party systems. Where it's broken / wrong, that's an obvious reason to fix it. But are there third party systems that will break if we set the version and command ID correctly?

@druckgott druckgott closed this by deleting the head repository Oct 5, 2024
@mmosca mmosca removed this from the 8.0 milestone Oct 24, 2024
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.

6 participants