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

use QSerialPort for garmin serial communication. #1279

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

tsteven4
Copy link
Collaborator

This moves OS specific code out of gpsbabel (taking advantage of OS specific code in QSerialPort). I have tested this with a GPS12 but I am unable to test the baudrate option with that device.

I have conversion of gpsdeviceh to a base class GpsDevice, with subclasses GpsSerialDevice and GpsUsbDevice. I am unable to test GpsUsbDevice at all.

Can we come up with a testing plan? Test in the GPSBabel lab? Relocate some devices to the GPSBabel west lab?

@tsteven4 tsteven4 marked this pull request as draft May 15, 2024 16:53
Copy link
Collaborator

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

I really like this.

I'm on the road and can't help test this, but agree that this probably needs some special attention.

jeeps/gpsserial.cc Outdated Show resolved Hide resolved

if (!GPS_Serial_Open((gpsdevh*) psd,port)) {
GPS_Error("Cannot open serial port '%s'", port);
auto* h = reinterpret_cast<serial_data_handle*>(dh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

All this funky casting of things into. a void* was necessary and hip when we were C. I know it's another shelf of cans of worms, but I wonder if we should just make serial_data_handle a "real" data structure and quit this funky casting.
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The casting almost entirely goes away in the next step, when GpsDevice becomes a base class with derived classes GpsSerialDevice and GpsUsbDevice. That obviously touches the usb communication, another thing I can't test, but if you really like what you have seen so far I think you will like that even more.

xfree(dh);
return 1;
/* Process IO */
(void) GPS_Serial_Chars_Ready(dh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this now be [[maybe_unused]]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No.

warning: attributes at the beginning of statement are ignored [-Wattributes]
  239 |   [[maybe_unused]] GPS_Serial_Chars_Ready(dh);


// Sleep for a small amount of time, about 100 milliseconds,
// to make sure the packet was successfully transmitted to the GPS unit.
QThread::usleep(100000);
QThread::usleep(100000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like Device_Flush and/or Wait should do something more appropriate for us. Can one of those maybe do a https://doc.qt.io/qt-6/qserialport.html#flush for us to do a TCSADRAIN or something and eliminate this?
I get that you only changed whitespace here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The original code looks wrong to me!
GPS_Device_Flush on unix calls tcflush(), which I think I correclty translated to QSerialPort::clear().
But tcflush discards data, it doesn't send it on its way.

tcflush() discards data written to the object referred to by fd but not transmitted, or data received but not read, depending on the value of queue_selector:

What GPS_18x_Tech_Specs.pdf says is to wait a small amount of time to make sure the previous packet (the ACK of IOP_BAUD_ACPT_DATA) is complete. So it sounds to me like we wanted to call tcdrain() instead of tcflush(), which would translate to QSerialPort::flush(). QSerialPort::flush() may not be necessary after QSerialPort::waitForBytesWritten.

I note that GPS_18x_Tech_Specs were updated 1/2024!

I cant test this as the GPS12 doesn't support baud rate changes.

Copy link
Collaborator

@robertlipe robertlipe May 17, 2024

Choose a reason for hiding this comment

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

ISTR that Garmin protocol is always 9600, Period. I'm not even sure why this is exposed.

The original code being wrong wouldn't shock me. I had to fix a LOT of stuff in Jeeps serial, maybe even before I checked it in the first time. My own fixes may not have been water-tight. I remember having concerns about the way data was shoved around synchronously in some places and more loosely in others. When I simulated really losing data and forcing protocol retries, it largely came unglued

return gps_errno;
}

if (global_opts.debug_level >= 1) fprintf(stderr, "Serial port speed set to %d\n", br);
if (global_opts.debug_level >= 1) {
fprintf(stderr, "Serial port speed set to %d\n", br);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we push this into a qDebug() or a Warning() or one of our other existing methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had difficulty preserving order when I mixed the qDebug messages with traditional fprintf messages jeeps used in GPS_Diag. For consistency perhaps this always should have used GPS_Error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, but it's not really an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I'm back after GeoWoodstock,. I'll retest with at least two USB protocol and two USB serial (perhaps using something like a 60csx that provides both)

I'll also look ask my geocaching friends what's in their hand-me-down drawer and buy/ship you some representative hardware if you like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a GPS12 that does 9600 serial only. I have no way to test baud rate changes. Baud rate changes were supported by some variants of the GPS 18. GPS 15xH/15xL had user-selectable baud: 4800, 9600, 19200, 38400. The return code from GPS_Serial_Set_Baud_Rate is also problematic, e.g. success and a tcsetattr error both return 0.

I don't have a usb device, so anything that could allow me to test gpsusb* would be useful as well. That would enable the next step - make GpsDevice an abstract base class - which gets rid of a bunch of casting and gpsdeviceops.

@robertlipe
Copy link
Collaborator

robertlipe commented May 18, 2024 via email

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