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 endpoint grouping #60

Open
wants to merge 9 commits into
base: auterion-router-latest
Choose a base branch
from

Conversation

DanielePettenuzzo
Copy link

@DanielePettenuzzo DanielePettenuzzo commented Jun 28, 2024

This endpoint grouping is designed for systems where a software component will send to and receive from mavlink router on two different endpoints. This is what we need for the auterion wfb setup with the wifi radios. We need to specify all ports we send to and receive from AMC. By giving both tx and rx endpoints the same group, mavlink router will just wait to receive the system/component ids on one of the two endpoints and then assign the ids to the entire group since one endpoint will just send out data and will never receive a system and component id.

This PR will then modify also the config generator script: https://github.com/Auterion/recipe-mavlink-router/pull/10 We still need to clean up this PR.

There might be something similar upstream but I would prefer to get this in and then finally plan the mavlink router upstream merge that we have been postponing from 3 years now.

FYI @FrauBluher @alexVinarskis

Copy link
Collaborator

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Why not use the implementation from upstream right away? mavlink-router@354a5f5
It's considerably simpler.

@@ -47,6 +47,10 @@

#define UART_BAUD_RETRY_SEC 5

// Static vars protected by a mutex, shared across all instances of Endpoint
std::mutex Endpoint::_group_sys_comp_ids_mutex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No locking needed, mavlink-router is single-threadded

@alexVinarskis
Copy link

@DanielePettenuzzo ?)

@alexVinarskis
Copy link

alexVinarskis commented Aug 8, 2024

re-visiting this
@FrauBluher could you please address @bkueng comments?
anything preventing from using upstream method then? will be able to do a quick cross-test if its implemented

this is last 'unmerged' package for the skynode-s. in an effort to clean up and stabilize the relaeses, i would really want to see this guy merged :D

@FrauBluher
Copy link

FrauBluher commented Aug 8, 2024

I briefly looked into using the upstream method though found it faster to implement this than to refactor the Endpoint class to look more like upstream's.

I'm of the opinion that if we're going to put any more effort into modifications to mavlink-router that we should just abandon this and add the watchdog kicker and named-pipe endpoint generation that the payload-manager uses to the upstream version of mavlink-router. I got auterion-wfb-ng working with upstream mavlink-router without issue, we just can't use it as-is until we add back:

  • watchdog changes
  • named pipe stuff
  • our logging output changes?
  • (what else am I missing)

This is roughly what @DanielePettenuzzo and I discussed previously.

@alexVinarskis
Copy link

thanks!
Sadly Daniele is still out this week, and thus i assume will be really busy early next week...
@bkueng are you the right person to discuss/decide on things Pavlo proposed? (sorry if not, just not sure who is here responsible for what exactly)

@bkueng
Copy link
Collaborator

bkueng commented Aug 12, 2024

I briefly looked into using the upstream method though found it faster to implement this than to refactor the Endpoint class to look more like upstream's.

Yes the structure is a bit different, and maybe I'm missing something but I still think the upstream implementation could easily be adopted and it's considerably simpler. You can prove me wrong though.

@bkueng are you the right person to discuss/decide on things Pavlo proposed? (sorry if not, just not sure who is here responsible for what exactly)

At this point I'm not in the loop anymore about specific differences.

@DanielePettenuzzo
Copy link
Author

@FrauBluher yes the diff you posted is pretty accurate. Those things can be resolved but will require a little bit of time from Matej or Andrew. We could potentially merge this and then plan to get rid or the dynamic endpoints or upstream them so that we can just move directly to the upstream version and not maintain our fork anymore.
@bkueng would you be fine in merging this implementation in our fork and then I put on the roadmap for the next month for someone to move to the upstream mavlink router?

@bkueng
Copy link
Collaborator

bkueng commented Aug 15, 2024

@bkueng would you be fine in merging this implementation in our fork and then I put on the roadmap for the next month for someone to move to the upstream mavlink router?

Yes if I have your word on that. And my comment still needs to be addressed: https://github.com/Auterion/mavlink-router/pull/60/files#r1662352785.

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