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

Add proper broadcast version check to support Crazyradio 2.0 broadcasting #35

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

whoenig
Copy link
Contributor

@whoenig whoenig commented Jul 27, 2023

Tested with ./example_broadcast

@whoenig
Copy link
Contributor Author

whoenig commented Jul 27, 2023

This still doesn't work with Crazyswarm2, likely because we use non-broadcasts and broadcasts mixed there (which we don't have tests for here). See IMRCLab/crazyswarm2#264 for a discussion. Essentially we get a LIBUSB Timeout error after trying to use broadcasts for the first time.

@knmcguire
Copy link
Member

Hi! Ah oke so it wasn't really working properly then after all... sorry, we thought it did! Would you like to fix that within this PR still while in draft? or is this version only about checking the version.

@whoenig
Copy link
Contributor Author

whoenig commented Jul 27, 2023

My goal is to get a standalone reproducible in this repo, which can be used to either fix it here or in the Crazyradio2 firmware.

@knmcguire
Copy link
Member

got it! Give us a headsup if you'd like this reviewed

@whoenig
Copy link
Contributor Author

whoenig commented Jul 27, 2023

I have added a reproducible for the issue:

  1. Build this repository
  2. Get a Crazyflie with propellers unmounted (or else stably fixed) that has URI radio://0/80/2M/E7E7E7E7E7.
  3. In the build folder run ./example_broadcast_and_console

Observed behavior:
Crazyradio PA: terminates without issue (shows console messages while also sending broadcast commands)

Crazyradio 2: triggers exception:

terminate called after throwing an instance of 'std::runtime_error'
  what():  LIBUSB_ERROR_TIMEOUT

Perhaps @ataffanel or @knmcguire can take a look? I suspect that this is a Crazyradio 2 firmware issue. The error comes directly from libusb, i.e., after sending a broadcast command the radio doesn't seem to be responsive to USB reads anymore (which are needed for the regular unicast communication).

@knmcguire
Copy link
Member

knmcguire commented Jul 27, 2023

Hmm okay! Perhaps that is maybe also the cause for the instability we had when trying broadcasts? Or perhaps we weren't using it at all.

Arnaud should be back on the 21th of August to help out but I'll give this a try as well in the next few days.

@knmcguire knmcguire closed this Jul 27, 2023
@knmcguire knmcguire reopened this Jul 27, 2023
@williamleong
Copy link

Hmm okay! Perhaps that is maybe also the cause for the instability we had when trying broadcasts? Or perhaps we weren't using it at all.

Arnaud should be back on the 21th of August to help out but I'll give this a try as well in the next few days.

Hi @knmcguire, would you be able to elaborate on the instability when using broadcasts? We experienced some cases during the initialization of Crazyswarm2 where the initialization would get stuck after a broadcast was sent (it was something like [all] radiobroadcast://......).

@knmcguire
Copy link
Member

@williamleong I think it would be good to leave that discussion for the Crazyswarm2 discussions ;) There, I thinkt @whoenig can better answer your question, as I'm more experienced with the cflib backend of Crazyswarm2 that does not support broadcasting

@knmcguire
Copy link
Member

knmcguire commented Jul 28, 2023

I've tried the examples on both crazyradio 1 and 2 and I'm not seeing any difference and no error whatsoever... full console output on both.

I had a fresh CR2 which I reflashed by means of the tutorial.

let me see if I can find any other one

@knmcguire
Copy link
Member

nope... no problem with to see with the others as well.. is this more of a usb driver issue you think or?

@whoenig
Copy link
Contributor Author

whoenig commented Jul 28, 2023

Very interesting. I also just used a brand new CR2 and followed the tutorial. Are you running on Ubuntu or WSL? I used Ubuntu 22.04 with the system libusb. I could certainly also try to use a self-build libusb (on Windows we already do that), if we suspect it's a USB driver issue.

@knmcguire
Copy link
Member

I've tried it on Ubuntu 22.04 myself with libusb and the devrules. The cmake build on my windows is a bit messed up so I can't try it there yet.

@knmcguire
Copy link
Member

btw, which version of libusb do you have? I have 1.0.25 on my ubuntu

@whoenig
Copy link
Contributor Author

whoenig commented Jul 28, 2023

I have 1.0.25 and yes, just the normal udev-rules.

What kind of machine are you using? Is it connected over USB3 ports? I tried on a Thinkpad P14s with the radios directly plugged in to one of the USB-A ports (USB3.2 or so). I haven't done many repeated attempts, but I could check if other ports or anything change the behavior from my side.

@williamleong
Copy link

I was running Ubuntu 20.04 with the system libusb which looks like 1.0.23, when I encountered the error.

@whoenig
Copy link
Contributor Author

whoenig commented Jul 28, 2023

@williamleong, you mean the error in Crazyswarm2, correct? Or did you also try the standalone example provided in this PR?

@williamleong
Copy link

@williamleong, you mean the error in Crazyswarm2, correct? Or did you also try the standalone example provided in this PR?

@whoenig that's right, I was referring to the error when we used the Crazyradio 2.0 in Crazyswarm2.

@knmcguire
Copy link
Member

@whoenig I've tried it directly on a port of my laptop, which is also USB 3.2 as well. But I've only tried one.

seems like a tricky thing to figure out what our differences is 😅. If you have another crazyradio 2.0, perhaps try that out just to eliminate any hardware issue with it?

@whoenig
Copy link
Contributor Author

whoenig commented Aug 2, 2023

I did more experiments:

  1. Same results with different USB port
  2. When using sleep rather than yield in https://github.com/bitcraze/crazyflie-link-cpp/blob/master/src/CrazyradioThread.cpp#L131-L132 (something that helped in Does not work with 4 crazyradios IMRCLab/crazyswarm2#263), the behavior changes. The timeout is gone, but safelink does not get activated consistently and broadcasts don't seem to be actually sent consistently.
  3. More experiments with Crazyradio PA. It works more frequently, but certainly not always. It also seems to starve the broadcasts frequently.
  4. I applied the prioritization that Arnaud doesn't like (Prioritize broadcast commands #30), which we also use in Crazyswarm2. It improves the situation for Crazyradio PA, but not for Crazyradio2.

Overall, I think we could merge this PR, but we have to create a new issue that investigates why the new example doesn't work consistently. At least it seems mostly an issue with crazyflie-link-cpp, and not the Crazyradio2 firmware.

@whoenig whoenig marked this pull request as ready for review August 2, 2023 05:40
@whoenig
Copy link
Contributor Author

whoenig commented Aug 2, 2023

If you want I can also revert the new example (commit 9b4e72d). I personally don't care that much since I would like to have this example and if it's part of this merge or not doesn't matter too much to me.

@whoenig
Copy link
Contributor Author

whoenig commented Aug 2, 2023

And some more updates: I had the channel set incorrectly. With all patches (sleep rather than yield, PR30, this PR), everything seems OK for both Crazyradio PA and Crazyradio2. For the sleep vs. yield I'll need Arnaud's help - I'll create a new issue and tag him (see #37).

@tobbeanton tobbeanton merged commit 3a8c922 into master Aug 2, 2023
@tobbeanton tobbeanton deleted the crazyradio20 branch August 2, 2023 09:01
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.

4 participants