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

feat: Switch serial library to one with larger cross-platform support #138

Merged
merged 11 commits into from
Dec 15, 2024

Conversation

chrisdalke
Copy link
Contributor

@chrisdalke chrisdalke commented Dec 15, 2024

Overview

Change the serial library from github.com/tarm/serial to go.bug.st/serial. These serial libraries seem to have almost equivalent APIs, but go-serial has support for a much larger set of platforms.

This change should make it easier to use gomavlib on a wider variety of platforms, and is to support a downstream PR I'm making to Telegraf (influxdata/telegraf#16221), a popular Go agent for data ingestion.

Resolves #130.

@chrisdalke chrisdalke changed the title Switch serial library to one with larger cross-platform support feat: Switch serial library to one with larger cross-platform support Dec 15, 2024
@aler9
Copy link
Member

aler9 commented Dec 15, 2024

Thanks for this patch, you have to know that i also performed this change some time ago in order to support hardware flow control, but i was forced to stop due to lack of feedback from the person who reported the issue in the first place, as i do not own required hardware :D

This was my work:
https://github.com/bluenviron/gomavlib/tree/archived/serial-hfc

Please check whether there's something that can be integrated into this patch - then i'll merge everything.

@aler9
Copy link
Member

aler9 commented Dec 15, 2024

In particular: can you use these instructions for initializing the library?

	dev, err := serial.Open(device, &serial.Mode{
		BaudRate: baud,
		Parity:   serial.NoParity,
		DataBits: 8,
		StopBits: serial.OneStopBit,
	})
	if err != nil {
		return nil, err
	}

	dev.SetDTR(true) //nolint:errcheck
	dev.SetRTS(true) //nolint:errcheck

and can you report whether they are working with your hardware? in particular the last two lines.

@chrisdalke
Copy link
Contributor Author

chrisdalke commented Dec 15, 2024

In particular: can you use these instructions for initializing the library?

	dev, err := serial.Open(device, &serial.Mode{
		BaudRate: baud,
		Parity:   serial.NoParity,
		DataBits: 8,
		StopBits: serial.OneStopBit,
	})
	if err != nil {
		return nil, err
	}

	dev.SetDTR(true) //nolint:errcheck
	dev.SetRTS(true) //nolint:errcheck

and can you report whether they are working with your hardware? in particular the last two lines.

Updated the PR to use this. Tested that the serial connection works fine with a USB-to-UART connection to an ArduPilot flight controller, however the linux USB serial driver just ignores all these settings. To test the RS232 hardware flow control needs a Windows compute, RS232 adapter, and oscilloscope or logic analyzer to look at the DSR and RTS pins on the physical connector.

Most drivers will ignore these flags if they are not relevant so this is likely safe to set on all ports even if hardware flow control is not applicable.

@chrisdalke
Copy link
Contributor Author

Sorry, having trouble getting the linting to pass. It seems like maybe the linter executed by make format disagrees with golangci-lint?

@aler9 aler9 merged commit 8391bff into bluenviron:main Dec 15, 2024
5 checks passed
@chrisdalke chrisdalke deleted the cdalke/change-serial-library branch December 15, 2024 22:38
@aler9
Copy link
Member

aler9 commented Dec 15, 2024

i merged the patch, thanks for providing it. Good luck with your Telegraf patch, 6 years ago i wrote a service for logging flights, in which a gomavlib-based script was writing Mavlink data to InfluxDB. Data was made available to users through Grafana and this plugin:

https://grafana.com/grafana/plugins/pr0ps-trackmap-panel/

The result was pretty good, it turned out that tools coming from the IT observability world are a nice fit for drones too.

Regarding the patch, my advice is to switch to the mainline version of gomavlib rather than using a fork, otherwise it would be really difficult to propagate bug fixes.

@chrisdalke
Copy link
Contributor Author

Thank you! Yes, the PR on telegraf used the fork just temporarily. Now that this is merged I'm going to point it back to your library.

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.

Consider changing serial library to improve multiplatform support
2 participants