NTRIP client POC #6574
NTRIP client POC #6574
Conversation
|
@edgecase14 , first and foremost, this is a super nice contribution, you're making a very nice entry into the QField community this month :) I'll review the PR in a second. One big structural we'll need to change here is where the NTRIP communication is happening. We'll want this sitting at the NmeaGnssReceiver class instead of within the BluetoothReceiver class. One reason for that is that on Windows, Bluetooth connectivity often goes through virtual serial port where QField uses the serial port receiver to communicate with the GNSS device. That shouldn't be a huge change here, just a bit of code reshuffling around. |
| if ( firstByte == 0xD3 ) | ||
| { | ||
| qDebug() << "RTCM chunk:"; | ||
| } | ||
| else if ( firstByte == 0x73 ) | ||
| { | ||
| qDebug() << "SPARTN chunk:"; | ||
| } | ||
| else | ||
| { | ||
| qDebug() << "UNKNOWN chunk:"; | ||
| } | ||
|
|
||
| qDebug() << data.size() << "bytes"; | ||
| // send to your GNSS device |
There was a problem hiding this comment.
Let's clean some of that debug prior to merging so we don't get too verbose when running QField in debug mode.
| //connect(mReply, &QNetworkReply::readyRead, this, &NtripClient::onReadyRead); | ||
| //connect(mReply, &QNetworkReply::finished, this, &NtripClient::onFinished); | ||
| //connect(mReply, xxxQOverload<QNetworkReply::NetworkError>::of(&QNetworkReply::errorOccurred), | ||
| // this, &NtripClient::onError); |
| { | ||
| if ( mReply ) | ||
| { | ||
| disconnect( mReply, nullptr, this, nullptr ); // ✅ Disconnect all signals |
There was a problem hiding this comment.
No emojis in source file please :)
|
|
||
| if ( mReply->isRunning() ) | ||
| { | ||
| mReply->abort(); // ✅ Cancel the request |
| /* | ||
| void NtripClient::onReadyRead() | ||
| { | ||
| QByteArray data = mReply->readAll(); | ||
| qInfo() << data + "\n"; | ||
| emit correctionDataReceived(data); | ||
| } | ||
| */ |
| else | ||
| { | ||
| // Wait for more data | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
When you read all the data here, the buffer will be gone, meaning that as your waiting for more data, you'll lose the data that came before, is there a need to preserve the data to append it to the next frame until the \r\n\r\n bit arrives?
| connect( mPositioningSourceReplica.data(), SIGNAL( ntripBytesSentChanged() ), this, SIGNAL( ntripBytesSentChanged() ) ); | ||
| connect( mPositioningSourceReplica.data(), SIGNAL( ntripBytesReceivedChanged() ), this, SIGNAL( ntripBytesReceivedChanged() ) ); |
There was a problem hiding this comment.
I don't think we'll need those exposed here. Is that an important part to you, and if so, how would this be used?
| @@ -66,6 +66,15 @@ class Positioning : public QObject | |||
| Q_PROPERTY( QString loggingPath READ loggingPath WRITE setLoggingPath NOTIFY loggingPathChanged ) | |||
|
|
|||
| Q_PROPERTY( bool backgroundMode READ backgroundMode WRITE setBackgroundMode NOTIFY backgroundModeChanged ) | |||
| Q_PROPERTY( bool enableNtripClient READ enableNtripClient WRITE setEnableNtripClient NOTIFY enableNtripClientChanged ) | |||
There was a problem hiding this comment.
To fit with other property names here, I'd rename that to:
bool ntripCorrection READ ntripCorrection WRITE setNtripCorrection NOTIFY ntripCorrectionChanged )
| signals: | ||
| void correctionDataReceived( const QByteArray &data ); | ||
| void errorOccurred( const QString &message ); | ||
| void streamConnected(); |
There was a problem hiding this comment.
We should have a streamDisconnected signal to allow for the code to react to a server-induced disconnection.
| Q_PROPERTY( QString ntripMountpoint READ ntripMountpoint WRITE setNtripMountpoint NOTIFY ntripMountpointChanged ) | ||
| Q_PROPERTY( QString ntripUsername READ ntripUsername WRITE setNtripUsername NOTIFY ntripUsernameChanged ) | ||
| Q_PROPERTY( QString ntripPassword READ ntripPassword WRITE setNtripPassword NOTIFY ntripPasswordChanged ) | ||
| Q_PROPERTY( QString ntripStatus READ ntripStatus NOTIFY ntripStatusChanged ) |
There was a problem hiding this comment.
Let's use an enum here, and rename it to state (Connected, Disconnected for now?). This will allow us to then more easily localize the state is we want it reflected in the UI itself.
For error messsages, I'd adopt a QString ntripLastError property (to match preexisting deviceLastError property).
|
@edgecase14 , following up on this, are you planning to push this exciting PR further? :) |
|
Yes, just other work came up. I am new to github PR reviews if there's a quickstart guide you can point me to great otherwise I'll muddle my way through. |
|
@edgecase14 , happy to hear you are forging ahead! :) Let me know if anything I said in my individual review comments was not clear. You should insure that the pre-commit hook is installed locally (should just be a matter of typing pre-commit install in your terminal provided you have the pre-commit python package already installed on your system). |
Blindly stuffs TCP packets received over Bluetooth. RTCM or SPARTN. TCP message fragments aren't reassembled.
Tested on Linux, with PointPerfect(thingstream) and SparkFun RTK Surveyor firmware on custom harware. RTK Fixed is achieved.
See #5387. Comments requested. Android testing encouraged.