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

Broadcast support & examples #9

Merged
merged 6 commits into from
May 28, 2021
Merged

Broadcast support & examples #9

merged 6 commits into from
May 28, 2021

Conversation

mikendu
Copy link
Contributor

@mikendu mikendu commented May 5, 2021

No description provided.

Copy link
Contributor

@whoenig whoenig left a comment

Choose a reason for hiding this comment

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

This looks good overall. I really like the PacketHelper and the more extensive examples on how to use the library. My main concern is the URI - I do agree that your version is easier to parse (and that the current parsing code is a mess), but the URI design was carefully crafted to be backward compatible as well as unified across backends. We'll need @ataffanel input before we can finalize the broadcast URI.

CMakeLists.txt Outdated Show resolved Hide resolved
src/Connection.cpp Show resolved Hide resolved
src/CrazyradioThread.cpp Outdated Show resolved Hide resolved
Comment on lines 82 to 84
// Fixes a crash where device gets destroyed before it gets unref'ed
crazyradios_.clear();

Copy link
Contributor

Choose a reason for hiding this comment

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

neat - thanks!

@mikendu mikendu requested a review from whoenig May 10, 2021 18:52

const std::regex uri_regex("(usb:\\/\\/(\\d+)|radio:\\/\\/(\\d+|\\*)\\/(\\d+)\\/(250K|1M|2M)\\/([a-fA-F0-9]+|\\*)(\\?[\\w=&]+)?)");
const std::regex uri_regex("(usb:\\/\\/(\\d+)|radio(broadcast)*:\\/\\/(\\d+|\\*)\\/(\\d+)\\/(250K|1M|2M)(\\/([a-fA-F0-9]+|\\*)(\\?[\\w=&]+)?)*)");
Copy link
Contributor

Choose a reason for hiding this comment

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

This regex now allows to have address and URI flags for the radiobroadcast backend as well, e.g., radiobroadcast://0/80/2M/E7E7E7E7E7?ackfilter=0 would still be accepted. I think it would be cleaner to add a new option, i.e., "(usb:\\/\\/(\\d+)|radio:\\/\\/(\\d+|\\*)\\/(\\d+)\\/(250K|1M|2M)\\/([a-fA-F0-9]+|\\*)(\\?[\\w=&]+)?)|radiobroadcast:\\/\\/(\\d+|\\*)\\/(\\d+)\\/(250K|1M|2M)". Not pretty, but better error handling & reporting for malformed URIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, fair point 👍

@mikendu mikendu requested a review from whoenig May 16, 2021 21:44
#include <thread>

#include "crazyflieLinkCpp/Connection.h"
#include "PacketUtils.hpp";
Copy link
Contributor

Choose a reason for hiding this comment

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

CI complains about the semicolon at the end of the line. Sorry, this wasn't enabled before, since you are a "new contributor", but CI should run any fix you push. I am happy to merge once the CI passes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, okay, no worries!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've pushed an update, but it looks like I'll need your approval for the workflows to run again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stepped away from this for a bit, but coming back I see the CI build is still failing. Currently it's failing with:

LINK : fatal error C1047: The object or library file '3rdparty\libusb-1.0.24\VS2019\MS64\static\libusb-1.0.lib' 
was created by a different version of the compiler than other objects like 'cflinkcpp.dir\Release\python_bindings.obj'; 
rebuild all objects and libraries with the same compiler [D:\a\crazyflie-link-cpp\crazyflie-link-cpp\build\temp.win-amd64-3.8\Release\cflinkcpp.vcxproj]

This is actually something I dealt with when developing locally, and I never found a fix for it (I just cloned the libusb repository, and built the lib from the source myself). Any thoughts on this? Maybe it's just a matter of updating to a newer build of libusb?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought I had merged this already as it looked good. The error you are seeing is unrelated to your changes and tracked in issue #12. We'll take a look ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no worries, awesome, glad this got merged in!

@whoenig whoenig merged commit 5a51baf into bitcraze:master May 28, 2021
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.

2 participants