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

Added support in Mavlink Ethernet channel parameters #14460

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

garfieldG
Copy link
Contributor

Mavlink Ethernet channel settings such as UDP port, remote port and broadcast mode now can be changed dynamically via parameters.

Describe problem solved by this pull request
-The start of the Mavlink Ethernet channel is static and not dynamic like the start of the serial channel.
-It allows us to dynamically choose Mavlink Ethernet channels settings via parameters.
-It allows us to use Broadcast on one Mavlink channel and Multicast on another.

Describe your solution
I added Ethernet related parameters to Mavlink.
Just like the serial parameters we had, the new parameters are times the number of Mavlink instances and are generated.
The new parameters will be generated only if the board supports Ethernet (by adding ETHERNET to its cmake).
Also, to choose Ethernet a new value was added to MAV_{I}_CONFIG and will be available only if the board supports Ethernet (otherwise the new value won't be generated as an option).

Additional context
I tested it on a costume board and it works fine.
Previously I suggested adding parameters such as IP address, Mac address.
In my code, I also added parameters to choose remote multicast address and another line in the generated Mavlink command that supports multicast, however, the added line only works if the multicast address is known from the parameters. I will gladly add it if you want.

@dagar
Copy link
Member

dagar commented Mar 24, 2020

Slightly lazy question, how does this extend multiple instances? For example on SITL (or a linux board) where we have multiple instances on different ports. Does specifying the board has ETHERNET give you one additional (parameter configurable and autostarted) Mavlink instance?

@dagar
Copy link
Member

dagar commented Mar 24, 2020

@dinomani this might be of interest for v5x.

@garfieldG
Copy link
Contributor Author

@dagar Hi, I'm not sure that I understand your question, but I'll try to answer.
It doesn't give any extra instances, you still have the same number of possible Mavlink instances, but you are now able to configure any of these Mavlink instances to use ethernet instead of serial via parameters.
When you specify your board has ETHERNET :

  • ethernet is added as a value that you can select :
    image
    you can choose ethernet on one instance or more.
    not like the other values, ethernet can be chosen multiple times.
    -new parameters are added :
    image
    image
    image

note that the extra parameters and value are to all instances and not just to instance 0.

one more thing, this change is not only for Mavlink, if in the future there will be any parameters that you would like to generate only for boards that support ethernet then you can always do that by using the ethernet keyword inside the yaml file.

@TSC21 TSC21 requested a review from dinomani April 4, 2020 08:05
@garfieldG
Copy link
Contributor Author

@dagar @dinomani
Hi,
Is there anything new regarding this PR? Would you like me to add or change something?

@BazookaJoe1900
Copy link
Member

@dagar can you take a look on that?

@dagar
Copy link
Member

dagar commented Jun 9, 2020

Sorry, I don't recall where we got to with this one.

Having configurable ports per mavlink instance feels wrong, but we can live with it. What if I wanted to have more than 1 mavlink instance (eg SITL)?

@dagar dagar added this to the Release v1.12.0 milestone Jun 9, 2020
@dagar dagar self-requested a review June 9, 2020 18:23
@garfieldG
Copy link
Contributor Author

garfieldG commented Jun 11, 2020

@dagar

Having configurable ports per mavlink instance feels wrong

I'm not sure I agree with you about that. I did it because we encountered a problem in which we needed more than one mavlink instance and each one of the instances needed to be working with a different port. It needed to be configurable because our need changed from time to time so we wanted it to be parameters configurable with no need to change something in the code each time just for a different port.

What if I wanted to have more than 1 mavlink instance

No problem here. Just like it was before you have MAV_{i}CONFIG. You add mavlink instances just like you did it before, the only change is that now you can select that your mavlink instances will work with ethernet instead of serial. you can have more than one ethernet instance, because unlike the regular serial options you can select in MAV{i}_CONFIG the ethernet option can be selected multiple times, if you chose it once you can choose it again as many times that you want (and as many max instances you have of course). Also, the ethernet option will only appear as a possible option only if your board supports ethernet (PX4_ETHERNET).

@BazookaJoe1900
Copy link
Member

@dagar ping....

@hamishwillee hamishwillee added the Documentation 📑 Anything improving the documentation of the code / ecosystem label Jun 21, 2020
@hamishwillee
Copy link
Contributor

If this goes in it may need documentation. Probably and update to https://docs.px4.io/master/en/peripherals/mavlink_peripherals.html

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 4, 2020
@LorenzMeier
Copy link
Member

Could you rebase? Sorry that this didn't get pushed forward earlier.

@stale stale bot removed the stale label Jan 31, 2021
@garfieldG
Copy link
Contributor Author

@LorenzMeier Thanks for the update. Just rebased it now.

@LorenzMeier
Copy link
Member

Thanks!

@garfieldG
Copy link
Contributor Author

@LorenzMeier Hi, I rebased again and added the rc.mavlink_override script as discussed in the dev call.

Just as a side note, we use parameters to decide the mac address and IP address. In the past, it didn't seem to get too much appreciation, but I would gladly pr it again now if there's any interest.

@mrpollo
Copy link
Contributor

mrpollo commented Feb 24, 2021

@davids5 can you make sure that this falls into your plans for mavlink/ethernet networking support, and its aligned?

@davids5
Copy link
Member

davids5 commented Feb 24, 2021

@davids5 can you make sure that this falls into your plans for mavlink/ethernet networking support, and its aligned?

This is just the configuration and is not setting the nodes's IP address. So good to go from my perspective.

@garfieldG - Would you please rebase this, so we can test it and merge it.

Mavlink Ethernet channel settings such as udp port, remote port and broadcast mode now can be changed dynamically via parameters.
@garfieldG
Copy link
Contributor Author

@davids5 rebased it, thanks!

@davids5 davids5 enabled auto-merge (rebase) February 25, 2021 10:37
@davids5
Copy link
Member

davids5 commented Feb 25, 2021

@garfieldG - Thank you, I have set it to auto merge. But if CI fails ping me.

@garfieldG
Copy link
Contributor Author

@davids5 The CI failed

@mrpollo
Copy link
Contributor

mrpollo commented Feb 25, 2021

I suspect the failures have nothing to do with this PR, I restarted the jobs just to be sure, but the errors were mostly segmentation faults and timeouts.

I will monitor the results of CI and merge if they are indeed false positives.

@@ -211,6 +224,80 @@ def parse_yaml_parameters_config(yaml_config):
param_group = parameters_section.get('group', None)
for param_name in definitions:
param = definitions[param_name]
if 'ethernet' in param:
Copy link
Member

Choose a reason for hiding this comment

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

Can you avoid the code duplication here, and instead pass has_ethernet_support?

@@ -339,7 +428,8 @@ def parse_yaml_parameters_config(yaml_config):
'port_param_name': port_param_name,
'default_port': default_port,
'param_group': port_config['group'],
'description_extended': port_config.get('description_extended', '')
'description_extended': port_config.get('description_extended', ''),
'ethernet_config': serial_command.get('ethernet', 'none')
Copy link
Member

Choose a reason for hiding this comment

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

'ethernet_config': serial_command.get('ethernet', None)

@@ -206,5 +210,9 @@ parameters:
# [0, N-1]
# Default: 0
type: integer
ethernet:
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to requires_ethernet: type: boolean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants