Length of sentence check fix

[tomy983]

Increased _NMEA_MIN_LENGTH, as it is pointless to send to NTRIP server a string without position info like this: $GNGGA,160653.60,,,,,0,00,99.99,,,,,,*79
Also made sure that maximum and minimum sentence check is performed excluding \r or \n

[robbiefish]

Thanks so much for contributing these changes! I will test them out ASAP, and get back with some more comments. In the meantime, there are a couple of changes to code style and other things I would like to see changed before this is merged

[tomy983]

Most of the changes have been made as requested. Some of those I am not really sure how to address them (for example the max nmea length...) Maybe adding a parameter... But this would probably generate more confusion than is worth.

[tomy983]

Hi, I should have the variables names cleaned now.
I also, as we discussed, added the option for the maximum nmea length # I've tested the code and (for me) is working no problem). I am throttling the nmea messages coming in from the gps as not to flood the ntrip server, limit data usage and also limit warnings log in case of bad nmea strings.
Apart from this, It's working alright.

[robbiefish]

Apologies that I have left this open for so long. I finally got the chance to test things out, and the NMEA sentence length change appears to work as expected. However, the reconnect logic does not appear to be working properly. When using a network (virtual) mountpoint, the driver will keep attempting to reconnect until the user sends a NMEA sentence

I think that the right thing to do here is to either update this PR to do just what the title suggests (adjustable NMEA length) or make a new PR with just the NMEA sentence length changes that way we can get it merged in without being held up on the reconnect issues.

I also think that the message package switching is implemented properly but should also probably be split into it's own PR. If you don't have the time, I am happy to make the PRs for you, just want to give you the chance to get the Github credit :)

Also, I took a look at the connection logic, and adapted your code to a different branch. If possible, please let me know if this reconnect logic also solves the reconnect problem for you: ROS, ROS2

[tomy983]

Apologies that I have left this open for so long. I finally got the chance to test things out, and the NMEA sentence length change appears to work as expected. However, the reconnect logic does not appear to be working properly. When using a network (virtual) mountpoint, the driver will keep attempting to reconnect until the user sends a NMEA sentence

IIRC it tries to reconnect every time a nmea message to be sent to the ntrip arrives on his topic. Id does not happens to me because I throttle the nmea topic like this:
<node name="nmea_throttler" type="throttle" pkg="topic_tools" args="messages /gps_link/nmea 0.1 /ntrip_client/nmea" />
I do this for two reasons:

• To avoid flooding the ntrip server with my position (some servers do not react well and some of them specifically ask for updates at least every n seconds. I read about a german user on a public ntrip who had complains for too frequent nmea updates. Can't find the reference now..)
• To save on data transfer (eventually I will connect via radio modem)
  I don't think there is any points in sending the nmea to the ntrip server so often (in my case it would otherwise be at 5Hz), so I throttle the message to publish every 10 seconds which for me is good enough for both update my position to the ntrip and also keeping the connection with it alive.
  When a new nmea message is received it is sent to the ntrip server. If the connection is lost it will attempt to reconnect.
  This (for me) also has the advantage that when testing inside and no valid nmea sentence is received (minimum lenght...) my ntrip server after about 60 sec disconnects me, but I will not attempt a reconnection until a valid nmea is published. When I move outside, a valid sentence is received and it will attempt a reconnection (every 10 seconds)

To recap: is the frequency of a valid nmea message that set the reconnection timing.
Btw, this would probably be a nice option to have in your code, instead of having to use topic_tools..

I think that the right thing to do here is to either update this PR to do just what the title suggests (adjustable NMEA length) or make a new PR with just the NMEA sentence length changes that way we can get it merged in without being held up on the reconnect issues.

It is not much code around that, would you mind just copy the part that you need?

I also think that the message package switching is implemented properly but should also probably be split into it's own PR. If you don't have the time, I am happy to make the PRs for you, just want to give you the chance to get the Github credit :)

That part of the code that deals with this is taken from here and you could create a pr from it.
Thank you for the consideration. I am grateful I have found your ntrip_client, @PaulBouchier adaptation was really useful so I used it and I just fixed what did not work for me. You absolutely go ahead and use whatever part you find useful as you wish.

Also, I took a look at the connection logic, and adapted your code to a different branch. If possible, please let me know if this reconnect logic also solves the reconnect problem for you: ROS, ROS2

Ok, I will give it a try.
I think we can close this pr.

[robbiefish]

Closing in favor of the following PRs:

• NMEA sentence length configuration:
  • ROS: ROS NMEA sentence min/max length #19
  • ROS2: ROS2 NMEA sentence min/max length #20
• Different RTCM message publishing:
  • ROS: ROS Adds ability to publish an rtcm_msgs Message instead of a mavros_msgs RTCM #22
  • ROS2: ROS2 Adds ability to publish an rtcm_msgs Message instead of a mavros_msgs RTCM #21
• Automatic reconnect:
  • ROS: ROS automatic reconnect #18
  • ROS2: ROS2 automatic reconnect #17