Repository navigation
Conversation
|
@meee1, here's the follow-up to PR #3752. I switched back to plain In the fallback case, however, the connected port is unknown and we configure several candidate ports ( As for the |
meee1
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. Several of the #3752 review points are handled here:
- When no prompt is recognised at a baud the receiver acknowledges, the legacy fallback to
DefaultPortsis used. At a wrong serial baud the acknowledgement fails and the scan moves on, and over TCP the scan no longer fails outright. - The prompt regex now matches
IP10>,IPS1>andBT01>as well asCOMx/USBx. activeSeptentrioPortstarts asnulland is reset on disconnect, so later RTCM changes no longer go to a previous receiver's port.SetEnabledRTCMstill falls back toDefaultPortsfornull, so this is safe.- Plain
RTCMv3on the detected port stops leftover SBF/NMEA streams using telemetry bandwidth.
One thing to fix before merge, inline:
- Corrections reach only the port the PC is on (
Septentrio.cs:102). This was point 3 of the #3752 review and is still open. Before #3752 the base sent RTCM toUSB1+USB2+COM1+COM2+COM3and set the baud on all three COM ports. With a detected port,setDataInOutandsetRTCMv3Outputnow target only that port, so a PC on USB1 with a telemetry radio on COM2 stops sending corrections to the radio with no error. COM3 is also dropped fromDefaultPortsand from the fallbacksetCOMSettings,COM1+COM2. Since #3752 is merged, this is a regression on master today, and this PR is the place to fix it.
Smaller things:
- Only USB has been tested. Serial at a non-default baud, TCP (the
IP10>case) and Bluetooth have not, so those paths are unverified on hardware. - No tests.
DetectPortandConfigureBaseReceiverare public and take anICommsSerial, so a fake port could cover the baud scan (a receiver starting at 9600, as suggested on #3752), the prompt regex and the fallback without hardware. - After trimming to the last 32 characters, the regex's
^matches at the cut point, so a mid-line fragment likeABC12>could be read as a prompt. That is unlikely withgecmreplies; trimming at a line break, or dropping^, would remove it. - The failing Build Release job is
Sysid32Tests.LiveLinkReadsFragmentedFramesAndTargetsWithoutAliasing, a timing-sensitive MAVLink test this PR does not touch. A re-run should clear it.
Review by Claude Code (Claude Opus 5.5, model claude-opus-5-5, medium effort) on behalf of @meee1, from the diff at 1360675 and the Septentrio code it calls, compared with the behaviour before #3752. Not built or run against a receiver.
🤖 Generated with Claude Code
| await SendAck(receiverPort, "setPVTMode,Static,All,Auto\n"); | ||
| // Append +RTCMv3 output to preserve any existing streams already configured on this port | ||
| await SendAck(receiverPort, $"setDataInOut,{targetPorts},Auto,+RTCMv3\n"); | ||
| await SendAck(receiverPort, $"setDataInOut,{targetPorts},Auto,{rtcmOutput}\n"); |
There was a problem hiding this comment.
With a detected port, targetPorts is that one port (for example USB1), so RTCM is output only where the PC is connected, and SetEnabledRTCM later sets the messages on that port alone. Before #3752 this was setDataInOut,USB1+USB2+COM1+COM2+COM3,Auto,RTCMv3, with setCOMSettings and setRTCMv3Output on the same five ports. A base whose corrections go to the drone through a telemetry radio on COM2, while the PC is on USB1, now stops sending them to the radio with no error.
COM3 is also gone: it is not in DefaultPorts (USB1+USB2+COM1+COM2) nor in the fallback setCOMSettings,COM1+COM2.
Suggested fix: keep sending RTCM to the full set including COM3, using plain RTCMv3 on the detected port as this PR does, or make the target ports a setting.
|
Thanks for the review. On the main point (RTCM only reaching the detected connected port), I covered this in #3752 (comment). In short:
I do see your point about a PC on USB1 and a telemetry radio on COM2: that radio no longer gets corrections from the auto-configuration, and there is no error. But this was part of the goal of #3752, sending RTCM only on the port Mission Planner is actually connected to, to avoid side effects on interfaces the user didn't ask to touch (leftover streams, other devices sharing the receiver, ports that don't exist on every model). If you'd still like to cover this case, I can append Good catch on the |
Follow-up to #3752, addressing the review comments on the Septentrio automatic RTK base configuration (port detection).
Changes
setCOMSettingsis tried andDefaultPortsis used when it succeeds.\b([A-Z]{2,4}\d{1,2})>at the start of a line, so descriptors such asIPS1,IPR1,NTR1andOTG1are detected, with no match inside other text.setCOMSettingsis only sent when the detected port starts withCOM(case-insensitive), then the port is accepted. Removed the unreachable comparisons and the case-sensitiveContains("COM").setDataInOutuses plainRTCMv3when the port is detected: Avoids leaving SBF/NMEA streams enabled on the connected port, which would use telemetry bandwidth alongside the corrections. On fallback toDefaultPorts, the connected port is unknown, so+RTCMv3is appended instead to avoid overriding streams that other ports may be using.activeSeptentrioPortreset on disconnect (and initialised tonull), so changing the RTCM level or constellations after connecting to another receiver or port no longer targets the old port.DetectPortbuffer trimmed to its last 32 chars (once it exceeds 64) so the regex doesn't rescan a growing string.Testing