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

Workaround for MSP Connections over BLE #7783

Closed
wants to merge 7 commits into from

Conversation

Scavanger
Copy link
Contributor

See #1441

Adds an MSP command so that the configurator can request that MSP frames are sent in chunks with defined size and delay, necessary for connection via some BLE modules.

@DzikuVx
Copy link
Member

DzikuVx commented Feb 22, 2022

@Scavanger to be honest, I'm skeptical about this change. As long as possible, the client should care about this. FC is doing everything like it should be doing.
I'd love to see BLE on Configurator, but Configurator should care not to demand too much data

@Scavanger
Copy link
Contributor Author

Scavanger commented Feb 22, 2022

@DzikuVx
Yes, I know, I am not happy with this solution either.
The problem is the Chrome BLE API and/or the Windows BLE driver in combination with BLE modules like the HM-10.
If a MSP frame is larger than the internal buffer (about 340 bytes), only the first 340 bytes arrive, or sometimes a part of the next frame arrives and then the rest of the large frame, which of course leads to CRC errors. This only affects a small part of the MSP commands (the Mixer tab is affected) and the CLI. There are no options in the chrome API to change this. This only applies to HM-1X modules, the SpeedyBee adapter runs without the workaround.
The only alternative BLE API might be Noble (https://github.com/noble/noble), but this would require a separate driver to be installed under Windows, which would disable Windows' own Bluetooth stack, so I don't think this is a solution.

@DzikuVx
Copy link
Member

DzikuVx commented Feb 22, 2022

@Scavanger which MSP frame overflows the 340 limit?

@Scavanger
Copy link
Contributor Author

@DzikuVx
MSP2_INAV_LOGIC_CONDITIONS

Payload length is 448 bytes + MSP header

@DzikuVx
Copy link
Member

DzikuVx commented Feb 22, 2022

Then maybe we shoult rework how this frame works. Because really, doing chunks on FW not on the serial driver level, is not the best idea

@Scavanger
Copy link
Contributor Author

Scavanger commented Feb 26, 2022

@DzikuVx
OK, I took a closer look at the whole thing again.
With 9600 baud it works without patches, everything else causes CRC errors and also an incorrect output of the CLI, especially with long outputs like dump and diff.

I now have three options:

  1. leave it like this and write a note in the documentation that BLE modules only work with 9600 baud (which at least 60% of the users won't read and then ask for help).
  2. make sure that no MSP frame is longer than 340 bytes, e.g. for MSP2_INAV_LOGIC_CONDITIONS the frame only contains one logic condition, the configurator must then send the index of the requested LC.
    A solution for the CLI still has to be found, a delay after each line would be necessary so that the adapter can empty its buffer, otherwise there will only be a jumble of characters.
  3. my solution from above ;)

@wx4cb
Copy link

wx4cb commented Feb 26, 2022

@DzikuVx OK, I took a closer look at the whole thing again. With 9600 baud it works without patches, everything else causes CRC errors and also an incorrect output of the CLI, especially with long outputs like dump and diff.

I now have three options:

  1. leave it like this and write a note in the documentation that BLE modules only work with 9600 baud (which at least 60% of the users won't read and then ask for help).

To be fair, it's been that way since the dawn of computers LOL... users will never RTFM that's why that whole phrase existed.

While i agree with you on this, IIRC BLE wasnt designed for heavy high speed links anyway, at least not on the serial port side.

  1. make sure that no MSP frame is longer than 340 bytes, e.g. for MSP2_INAV_LOGIC_CONDITIONS the frame only contains one logic condition, the configurator must then send the index of the requested LC.

i think this would be the better way to go about it especially as it's the only frame that is that long.

A solution for the CLI still has to be found, a delay after each line would be necessary so that the adapter can empty its buffer, otherwise there will only be a jumble of characters.

this will work and will be the easiest to implement, and of course, if we can bump the speed up even more eventually then this will get faster too.. i've not done much with BLE, but I am assuming that handshaking isnt going to be possible with something like XON/XOFF characters (the oldschool way) as i dont think there's anything RTS/CTS or DSR/DTR in BLE ?

@jbienz
Copy link

jbienz commented Feb 27, 2022

Just wanted to chime in with a few thoughts here.

write a note in the documentation that BLE modules only work with 9600 baud

Not only would they miss the note, but 9600 baud is really pretty low. Especially when most UARTs can handle 115200.

IIRC BLE wasnt designed for heavy high speed links anyway, at least not on the serial port side

BLE can achieve some surprisingly high speeds. This article talks about how to optimize for throughput and this article talks about theoretical maximums. Under ideal conditions it should be possible to write up to 305,084 bits per second to a GATT attribute - more than 2x the max speed of a serial COM port.

Also, don't forget that this is how the SpeedyBee App talks to iNav on iOS and Android.

make sure that no MSP frame is longer than 340 bytes, e.g. for MSP2_INAV_LOGIC_CONDITIONS

From my limited understanding so far, I agree that this seems like a better choice if possible. It's unfortunate that this really comes down to the Chrome BLE stack and the older / cheaper BLE modules.

I agree in theory that iNav Configurator shouldn't have any knowledge about the transport it's running on. But having a single tiny BLE device that can work with iOS and Android phones as well as full desktop configurator is incredibly valuable. IMO it's valuable enough for a less-than-tasteful software throttle if there's no other workaround. At least until the Chrome API or cheap BLE modules catch up.

Thanks for considering this contribution, and THANK YOU sincerely @Scavanger for all your hard work!

@MrD-RC
Copy link
Collaborator

MrD-RC commented Feb 27, 2022

  1. make sure that no MSP frame is longer than 340 bytes, e.g. for MSP2_INAV_LOGIC_CONDITIONS the frame only contains one logic condition, the configurator must then send the index of the requested LC.

I see a second advantage to this. It would also mean that we can increase the number of available logic conditions. Pilots are asking for more. I did some experimenting a while back, and above 40 (I believe it was) logic conditions, it would lock up Configurator. But looping through each condition, this limit no longer exists. At least because of the MSP frames.

One thing to be aware of. If this is to be done, it would be worth merging #7812 first (if the enhancement is wanted), as this changes the current MSP call.

@Scavanger
Copy link
Contributor Author

Scavanger commented Feb 27, 2022

@MrD-RC
Yes in any case. Other MSP commands (waypoints, safehomes) already work this way, only the logic conditions still send such "jumbo frames".

I have already modified the MSP command accordingly, so MSP via BLE already works.

Only with the CLI will we not be able to avoid tricking on the FC side. The BLE modules (HM-1X/HC-08/09) need, as I said, a short delay (20 - 30 ms) to empty their buffer, i.e. after each new line.
So there are two possibilities:

  1. a new CLI command (cli_delay <ms>) that activates the delay, the Configurator can, with a BLE connection, send the command automatically.
  2. a "CLI compatibility mode", which is started with a character other than '#' and thus signals the FC to send with the appropriate delay.

@jbienz
BLE via the modules works relatively slowly even with 115200 baud (whether 9600 or 115200 baud hardly matters), and the drop ratio is quite high, but there are at least no errors (CRC or timeouts).
Since it works faster with the Speedybee adapter, this is apparently a problem in the BLE/UART modules or in combination with the Web Bluetooth API (wich is still marked as experimental) and/or the Windows Bluetooth stack, but nothing that can be fixed in the Configurator. Unfortunately.

@stronnag
Copy link
Collaborator

stronnag commented Mar 2, 2022

I agree that this approach has some benefits:

  • Add a replacement MSP2_INAV_LOGIC_CONDITIONS that works like the current WP / Sethome implementations (i.e. one item per call) - futureproof adding for LCs.
  • Don't chunk in the firmware (if alleviated by replacing the MSP2_INAV_LOGIC_CONDITIONS as above.
  • Add the CLI delay directive. Allow this to be set within a range; currently 30 is hard-coded.

The final item has advantages beyond this particular use case:

  • Firmware built with gcc 11.2 works (does not "lock up" on CLI dump (even over VCP)).
  • Helps other (legacy) radio devices (3DR, HC-12, LoRa even)

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