feat(inputs.mavlink): Add plugin #16221
Conversation
telegraf-tiger
bot
added
the
feat
…to feature/mavlink-input-plugin
There was a problem hiding this comment.
@chrisdalke thanks for your contribution! Please check my comments in the code. Furthermore, I do have some general remarks
- I'm hesitant to merge plugins using forked one-man dependencies as experience shows that this is not maintainable on the long run. In your case I see two possibilities, the first being we hold-up the plugin until the upstream library merged your PR. This will likely take longer if possible at all.
The second option is to base your plugin on the upstream library and limit the availablility of the plugin to the platforms supported (excluding i386 and darwin if I understand correctly). What do you think? - Please keep the comments in the
sample.conffile brief and intend the file with two spaces. - Is there some way of mocking a Mavlink controller endpoint? E.g. using a minimal implementation mimicking some messages or a test-container/simulator?
Looking forward to your update!
plugins/inputs/mavlink/sample.conf
Outdated
| @@ -0,0 +1,39 @@ | |||
| # Read metrics from a Mavlink connection to a flight controller. | |||
There was a problem hiding this comment.
| # Read metrics from a Mavlink connection to a flight controller. | |
| # Read metrics from a Mavlink flight controller. |
plugins/inputs/mavlink/sample.conf
Outdated
| ## | ||
| ## The meaning of each of these modes is documented by | ||
| ## https://mavsdk.mavlink.io/v1.4/en/cpp/guide/connections.html. | ||
| fcu_url = "udp://:14540" |
There was a problem hiding this comment.
| fcu_url = "udp://:14540" | |
| url = "udp://:14540" |
|
|
||
| case *gomavlib.EventChannelOpen: | ||
| s.Log.Debugf("Mavlink channel opened") | ||
|
|
||
| case *gomavlib.EventChannelClose: | ||
| s.Log.Debugf("Mavlink channel closed") |
There was a problem hiding this comment.
Why do we need those? Should we do something about it if the channel is opened/closed?
There was a problem hiding this comment.
These messages indicate that the Mavlink connection has started/stopped, which is separate from the Mavlink client initializing its background thread.
There's nothing Telegraf can do in this case but it helps the user debug the Mavlink setup which can be difficult. One example of where this could occur is if you had two TCP client connections with the same system_id one would get kicked off. Another example is a flaky USB port.
Without these messages it's hard to tell the difference between (1) Mavlink is not connected vs (2) Robot has a problem and is not broadcasting messages
There was a problem hiding this comment.
Would it help to reconnect if the channel is closed?
srebhan
added
new plugin
plugin/input
srebhan
changed the title
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
I can prepare an ArduPilot container which will output data over UDP -- Is there a reference to how you'd like this setup to match other plugins? |
| The name of the Mavlink message is translated into lowercase and any | ||
| leading text `message_` is dropped. | ||
|
|
||
| For example, [MESSAGE_ATTITUDE](https://mavlink.io/en/messages/common.html) |
There was a problem hiding this comment.
| For example, [MESSAGE_ATTITUDE](https://mavlink.io/en/messages/common.html) | |
| For example, message [ATTITUDE](https://mavlink.io/en/messages/common.html#ATTITUDE) |
| ## Example Output | ||
|
|
||
| ```text | ||
| system_time,source=udp://:5760,sys_id=1 time_unix_usec=1732901334516981i,time_boot_ms=1731552i 0 |
There was a problem hiding this comment.
| system_time,source=udp://:5760,sys_id=1 time_unix_usec=1732901334516981i,time_boot_ms=1731552i 0 | |
| system_time,source=udp://:5760,sys_id=1 time_unix_usec=1732901334516981i,time_boot_ms=1731552i |
Including no timestamp is better than including a wrong one.
There was a problem hiding this comment.
Thanks @chrisdalke for the update and the explanations! Some more comments in the code, nothing too big...
Regarding testing, I suggest to also add a unit-test for convertEventFrameToMetric to make sure we get sensible metrics from different event types... Please use table-based testing for this (see this example) to allow adding new types easily.
For the integration test you can take some inspirations from this plugin. Let me know if you need help!
There was a problem hiding this comment.
As you are setting url, system_id, stream_request_enable and stream_request_frequency to some default value in the code I expect these to be sensible defaults. If that's the case, please comment the option in the config to let the user know that those are sensible defaults and are optional to change.
| stream_request_enable = true | ||
| stream_request_frequency = 4 |
There was a problem hiding this comment.
How about inverting the option so that false is the sensible default? Like
| stream_request_enable = true | |
| stream_request_frequency = 4 | |
| # disable_stream_requests = false | |
| # stream_request_frequency = 4 |
| // Split host and port, and use default port if it was not specified | ||
| host, port, err := net.SplitHostPort(u.Host) | ||
| if err != nil { | ||
| // Use default port if we could not parse out the port. | ||
| host = u.Host | ||
| port = "14550" | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is only relevant for UDP and TCP but not for serial?!? If so, please move the code down to the respective sections.
Furthermore, there is also the possibility of malformed host names like 127.0.0.1:: so you should check if the error is "missing port in address"...
| if len(host) > 0 { | ||
| s.endpointConfig = []gomavlib.EndpointConf{ | ||
| gomavlib.EndpointTCPClient{ | ||
| Address: host + ":" + port, | ||
| }, | ||
| } | ||
| } else { | ||
| s.endpointConfig = []gomavlib.EndpointConf{ | ||
| gomavlib.EndpointTCPServer{ | ||
| Address: ":" + port, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be simplified to
| if len(host) > 0 { | |
| s.endpointConfig = []gomavlib.EndpointConf{ | |
| gomavlib.EndpointTCPClient{ | |
| Address: host + ":" + port, | |
| }, | |
| } | |
| } else { | |
| s.endpointConfig = []gomavlib.EndpointConf{ | |
| gomavlib.EndpointTCPServer{ | |
| Address: ":" + port, | |
| }, | |
| } | |
| } | |
| s.endpointConfig = []gomavlib.EndpointConf{ | |
| gomavlib.EndpointTCPClient{ | |
| Address: host + ":" + port, | |
| }, | |
| } |
same for UDP.
| } | ||
|
|
||
| default: | ||
| return fmt.Errorf("could not parse url %s", s.URL) |
There was a problem hiding this comment.
Actually this should be
| return fmt.Errorf("could not parse url %s", s.URL) | |
| return fmt.Errorf("unknown scheme %q", u.Scheme) |
| URL: "udp://:14540", | ||
| FilterPattern: make([]string, 0), | ||
| SystemID: 254, | ||
| StreamRequestEnable: true, | ||
| StreamRequestFrequency: 4, |
There was a problem hiding this comment.
Setting default values should be done in Init()! Here you only need to keep values where the nil value is a valid setting.
| URL: "udp://:14540", | |
| FilterPattern: make([]string, 0), | |
| SystemID: 254, | |
| StreamRequestEnable: true, | |
| StreamRequestFrequency: 4, | |
| SystemID: 254, | |
| StreamRequestEnable: true, |
|
|
||
| filter filter.Filter | ||
| connection *gomavlib.Node | ||
| endpointConfig []gomavlib.EndpointConf |
There was a problem hiding this comment.
Why is this a list but you never set more than one config? Please change this to
| endpointConfig []gomavlib.EndpointConf | |
| endpointConfig gomavlib.EndpointConf |
and then pass [] gomavlib.EndpointConf{s.endpointConfig} to the connection setup instead.
| testConfig := Mavlink{ | ||
| URL: "serial:///dev/ttyACM0:115200", | ||
| } | ||
|
|
||
| err := testConfig.Init() | ||
| require.NoError(t, err) | ||
| endpoint, ok := testConfig.endpointConfig[0].(gomavlib.EndpointSerial) | ||
| require.True(t, ok) | ||
| require.Equal(t, "/dev/ttyACM0", endpoint.Device) | ||
| require.Equal(t, 115200, endpoint.Baud) |
There was a problem hiding this comment.
Please be brief! It also helps readability to call the plugin instance plugin... ;-)
| testConfig := Mavlink{ | |
| URL: "serial:///dev/ttyACM0:115200", | |
| } | |
| err := testConfig.Init() | |
| require.NoError(t, err) | |
| endpoint, ok := testConfig.endpointConfig[0].(gomavlib.EndpointSerial) | |
| require.True(t, ok) | |
| require.Equal(t, "/dev/ttyACM0", endpoint.Device) | |
| require.Equal(t, 115200, endpoint.Baud) | |
| plugin := &Mavlink{ | |
| URL: "serial:///dev/ttyACM0:115200", | |
| } | |
| require.NoError(t, plugin.Init()) | |
| endpoint, ok := plugin.endpointConfig.(gomavlib.EndpointSerial) | |
| require.True(t, ok) | |
| require.Equal(t, "/dev/ttyACM0", endpoint.Device) | |
| require.Equal(t, 115200, endpoint.Baud) |
Same for the tests below.
| ) | ||
|
|
||
| // Convert a Mavlink event into a struct containing Metric data. | ||
| func convertEventFrameToMetric(frm *gomavlib.EventFrame, msgFilter filter.Filter) telegraf.Metric { |
There was a problem hiding this comment.
I suggest moving this function to mavlink.go and get rid of the extra file. What do you think?
| return sampleConfig | ||
| } | ||
|
|
||
| func (s *Mavlink) Init() error { |
There was a problem hiding this comment.
Why is the variable named s? We usually use the first letter of the plugin type...
Summary
Add a Mavlink input plugin.
The
mavlinkplugin connects to a MavLink-compatible flight controller such as as ArduPilot or PX4. and translates all incoming messages into metrics.The purpose of this plugin is to allow Telegraf to be used to ingest live flight metrics from unmanned systems (drones, planes, boats, etc.)
Telegraf is already often used on flight computers (eg a Raspberry Pi) to collect system metrics for drones and it would be valuable to extend this to also provide a convenient way to record flight telemetry.
TODO
gomavlibis currently on a fork to support reading serial ports on i386 and darwin. Will need to upstream the fix for this and move back to the main repo after merge, or before merge if the forked gomavlib is not acceptableChecklist
Related issues
resolves #16220