search

amowry / WARBL2

WARBL2 Code and design files
https://warbl.xyz/index.html
GNU General Public License v3.0
5 stars 2 forks source link

Config tool: coexistence of the same WARBL connected via BLE and USB #10

Closed MrMep closed 1 day ago

MrMep commented 3 days ago

I guess this has already come up during the development of the firmware.

If a WARBL is connected BOTH via BLE and USB, sendConfig() is issued twice and the Config tool ends up in a mess. In other words, sendConfig is not thread-safe.

I can see at least four possible solutions (that don't mutually exclude):

  1. Implement a simple check when entering comm mode: if already active, we don't re-sendConfig(). We should make sure that comm mode is exited (the flag is set to 0) whenever ALL connections are lost: at the moment it doesn't look that way and I don't know if that's possible.
  2. Implement a semaphore. The problem is that the CC messages get intertwined, we should at least lock at the beginning of sendConfig and release at the end.
  3. Maybe simpler than 3 and a more general solution: the problem seems to be with multiple messages like jumpfactor and then value, or LSB/MSB, etc. We could "atomize" these couples of msgs and have a semaphore there.
  4. Use a different MIDI channel for BLE. This shouldn't be too hard, provided that we can tell the difference between BLE devices and USB devices in the config tool (of which I'm not too sure at the moment).

Of course, a temporary workaround is to manually disconnect either of the two.

Let me know if you want to address this problem and how, then if you want I can take care of it.

amowry commented 3 days ago

Yes, I was aware of this issue but never got around to dealing with it :).

This is probably aggravated by the fact that the WARBL sends messages to the Config Tool over both USB and BLE regardless of the selection in the WARBL2 Settings panel, which I did to prevent the user from getting locked out of the Config Tool when they change that setting.

It would be great of you wanted to address this (or I'm happy to as well), and I'm sure you know better than I do which method or combination of methods would be best, so I'm fine deferring to your expertise. Option 1 might be simplest (check whether the first Config Tool command comes over USB or BLE and then only send commands back to that connection?). You're right, the WARBL currently never exits communication mode unless explicitly told to do so. I expect we could just exit comm mode if the BLE connection is lost-- I assume the USB connection would only be lost when the WARBL is unplugged, in which case it powers down.

I guess one thing to be aware of is that someone might be using a BLE dongle that appears to the Config Tool as a USB device, so theoretically the WARBL could appear to the Config Tool as two different USB devices. That might cause issues for option 4? So maybe an implementation entirely from the WARBL end is best?

MrMep commented 2 days ago

Thanks Andrew. This is slightly more complicated than my initial analysis. (A huge) part of the problem is that the WARBL talks to itself, because of bi-directional CCs... ~~I have already implemented and tested solutions 2. and 3., but that doesn't resolve the problem. I don't see any viable solution apart from splitting channels in/out.~~

From the config tool p.o.v. that shouldn't be that complicated. ~~Let's say that Config tool -> WARBL : channel 7 WARBL -> Config Tool: channel 8 In the config tool should be sufficient to manage messages from channel 8 as well From WARBL it could be slightly more complicated. I'll make some tests and report here.~~ EDIT: that is not true, unless you have a MIDI routing software on your device ;)

So maybe an implementation entirely from the WARBL end is best?

I agree, but we are not able to manage connections from (really) multiple WARBLs. I guess this doesn't look like a big problem, for now.

EDIT: Complication level 3 ;) The problem is (also? solely?) in the fact that cc msgs are sent via both USB and BLE, here:

https://github.com/amowry/WARBL2/blob/a90325da783a1973508743a3fd4cc2dfda353250/warbl2_firmware/Functions.ino#L3221-L3229

I'm working on tracking where the enter comm mode command comes from, and send config msgs there only. So, basically, whatever arrives last with an enter comm mode msg, gets served.

joebowbeer commented 2 days ago

so theoretically the WARBL could appear to the Config Tool as two different USB devices.

Or even two different BLE devices if the WARBL's USB cable is terminated with a USB2BLE dongle. This is more likely to be accidental, but it could happen.

While I'm commenting, and you're looking at the config tool, I wish it could migrate old settings, or at the least know that it can't and fail fast.

amowry commented 2 days ago

It would be possible to migrate the old settings but I haven't had time make that happen, sorry. The Config Tool should give you an error if you try to import WARBL1 settings to the WARBL2 or vice versa (I added that about a month ago).

amowry commented 2 days ago

I'm working on tracking where the enter comm mode command comes from, and send config msgs there only. So, basically, whatever arrives last with an enter comm mode msg, gets served.

Yes, I think that could be a good solution, maybe in combination with a separate MIDI channel if necessary? I guess one of these would have to call a different function so we know the source of the CC message?

BLEMIDI.setHandleControlChange(handleControlChange); MIDI.setHandleControlChange(handleControlChange);

MrMep commented 2 days ago

I guess one of these would have to call a different function so we know the source of the CC message?

Yes. Ok, the problem was all there.

Roughly, here's what happened:

  1. I had already implemented a mutex for double messages: I'm leaving that because it doesn't hurt, but it didn't solve this problem.
  2. I had implemented a split in/out channel for communication with the config tool: I'm removing it because it looks quite unuseful at the moment,.
  3. On the config tool side, I did this: Instead of sending an Enter comm mode message to all devices, it sends one then waits for an answer. If a device responds (with firmware version), it stops sending the Enter comm mode msgs, otherwise tries the next device. This works well via web, but I had to bypass the messages queue. A few tests are required, I suppose, especially with older firmware versions and the app. This avoids the problem when settings are sent multiple times from (potentially) different WARBLs (or transports).
  4. On the WARBL side, we keep track of the transport (BLE or USB) that initiated the comm mode, and send config tool messages only through that channel. You'll see in the PR that I think I managed also the disconnections. A question: is the power down in case of USB disconnection hardware driven?

Once we get rid of the pending PRs, I'll submit a new one for this issue. Thank you.

amowry commented 2 days ago

Okay, thanks-- I'm not exactly sure what you mean by bypassing the messages queue, but it does seem to be necessary to have a delay between messages, which is the purpose of that queue. I'm not sure why it's necessary, but without it the WARBL2 was receiving messages in the wrong order and sometimes not at all.

The power down is basically hardware driven because the boost converter for the battery isn't normally on when there is USB power, so unplugging cuts the power to the device. It would be possible to enable the boost converter all the time which would allow the WARBL to immediately switch to battery power when unplugged, but I decided that would be confusing.

MrMep commented 1 day ago

It would be possible to enable the boost converter all the time which would allow the WARBL to immediately switch to battery power when unplugged, but I decided that would be confusing.

Maybe we could keep track of different states

amowry commented 1 day ago

I think I'd prefer to keep the behavior the same at this point if possible-- I'm afraid it could get confusing if unplugging the WARBL doesn't always turn it off, because without a power indicator it's a little tricky to tell when it's turned on. It also keeps things slightly easier for monitoring the battery level if it always powers down when unplugged.