AP_GPS: added GPS_DRV_OPTIONS bit for forcing ublox GPS to 115200 #16007
Conversation
libraries/AP_GPS/GPS_Backend.h
Outdated
| @@ -79,6 +79,12 @@ class AP_GPS_Backend | |||
| return _last_itow; | |||
| } | |||
|
|
|||
| enum DriverOptions : int16_t { | |||
| UBX_MBUseUart2 = (1 << 0U), | |||
There was a problem hiding this comment.
Uh....
| UBX_MBUseUart2 = (1 << 0U), | |
| UBX_MBUseUart2 = (1U << 0), |
?
| @@ -79,6 +79,12 @@ class AP_GPS_Backend | |||
| return _last_itow; | |||
| } | |||
|
|
|||
| enum DriverOptions : int16_t { | |||
There was a problem hiding this comment.
This is an odd storage type.
There was a problem hiding this comment.
This was just to reflect the fact that we are using this against a AP_Int16 everywhere, so just trying to minimize casts/extensions.
There was a problem hiding this comment.
What are the specific problems we are trying to address here? This mostly just seems unfortunate to include the whole UBLOX_SET_BINARY_115200 blob, since it was previously out of the build... (And since it's known to break if you try and combine this with some other options).
It's more work but I suspect if we really want something like this we are better off adding an option that says "honor the SERIALx_BAUD parameter instead, and use that as the basis of setting the baud rate, rather then hardcoding just this one for now.
| @@ -79,6 +79,12 @@ class AP_GPS_Backend | |||
| return _last_itow; | |||
| } | |||
|
|
|||
| enum DriverOptions : int16_t { | |||
There was a problem hiding this comment.
This was just to reflect the fact that we are using this against a AP_Int16 everywhere, so just trying to minimize casts/extensions.
the problem with that is our blob handling, we construct msgs with the crc attached in NMEA and UBX form. We'd need to use general NMEA and UBX creation fns to support the configured baudrate. Maybe it is worth it? |
It probably is, as the baudrate problems will grow worse over time. The other advantage that springs to mind is that if you have specified the type to be just UBlox for example if we are generating the blob at runtime we can drop the unneeded blobs for other drivers, which results in a smaller blob we are sending to the GPS, and allows us to start getting meaningful configuration done quicker, and avoids sitting behind a long queue of pending blobs. |
|
@WickedShell see this PR: #17261 |
|
@tridge given your comment here #17261 (comment) is it fair to say we should wait a bit more till that's investigated? It sounds like it's not actually the baud rate anymore... |
|
We definitely need something like this. Anecdotally I believe F405 without DMA is sucking up CPU from other things and we have many F405 boards where there is essentially only one DMA enabled UART and so if you have something like CRSF that absolutely requires DMA you are a bit screwed. I'm not sure I like the option bit - seems like its hard to configure. Maybe you could change the meaning to "use serial baud rate if supported" and check whether the baud rate has actually been configured and if not just default to the current behaviour. |
|
have a user that is having problems with M8030 based GPS with new high baud rate....works in stable, not master or beta....here is the issue (#18252) showing some affected modules...will build him a master version with this merged and have him test... |
|
User tested this and GPS now works and can arm... |
|
Why isn't this a bit in |
because it isn't affecting the serial port. It is affecting the auto-detection logic in AP_GPS. |
that would mean we'd need to construct the UBLOX_SET_BINARY_XXX packet for arbitrary baud rates. That is possible, but is a much more complex patch. |
We are rapidly trending towards that actually being the simpler patch, but I do agree we don't need it yet.
There are some minimums that need to not be deviated from (IE we have some hard minimum floors, 9600 will never work for example). |
its the memory management with the allocated blob that will be a bit tricky. Perfectly doable, but as there are calls for this in 4.1.0 I'd like to keep it simple. |
Agreed. The good news is if we can change that more of the init code may be sharable for stuff like the SBF drivers, that have to construct their strings at runtime. I'd agree with deferring the change to later, especially if trying to race it into 4.1 |
|
Needs the param description update to match. |
this may help with some GPS modules
this may help with some GPS modules