ClemensElflein / open_mower_ros

Other
498 stars 122 forks source link

Fix NTRIP client getting people ip banned #107

Closed EtheriVR closed 1 month ago

EtheriVR commented 3 months ago

I noticed that I got IP banned from rtk2go after the basestation went down for a bit, and the log was full of rapid reconnect messages never hitting the limit.

DavidBoman commented 3 months ago

I had the same issue. Adjusting theese paramaeters is good start. Even beter would be som kind of exponential backoff on the retry logic.

ClemensElflein commented 3 months ago

This should either be configurable via environment variable or use exponential backoff, because for people with local base stations this will now just stop mowing for now reason, if reconnect fails 10x due to poor wifi.

DavidBoman commented 3 months ago

Agree, configurable behavior would be the absolute best way to go.

EtheriVR commented 3 months ago

This should either be configurable via environment variable or use exponential backoff, because for people with local base stations this will now just stop mowing for now reason, if reconnect fails 10x due to poor wifi.

I absolutely agree that it should be configurable! Though setting it to 9999 seems like an imperfect solution for bad wifi, and they should probably fix their wifi rather than people getting banned 😅

I'm not quite sure it's worth me setting up the whole development environment to make the change though considering I have a very basic understanding of how the system of configs and flags actually work but if you are willing to give some pointers I'm happy to give it a try

fallingcats commented 2 months ago

There is another thing that's at least equally important for not getting banned (which I discovered the hard way):

The ntrip client crashes when a rtk2go mountpoint isn't available at the moment (no idea why). But that means it will restarts instantly and ignore the reconnect interval. To combat that, <node> needs the respawn_delay attibute set to something other than 0, I'd suggest the same value as the reconnect interval if that config option is to be added.

DavidBoman commented 2 months ago

@fallingcats : yeah... That's exactly what happend to me..

EtheriVR commented 2 months ago

Updated to be configurable @ClemensElflein I think I did it right, and doing a quick test run "on the bench" it seems to work, but will not know if it's fully functioning until I get my main board fixed, let you know how it goes

Apehaenger commented 1 month ago

Hi EtheriVR,

yes, it is configurable now, but as far as I see, with your preferred settings.

I worry all non-RTK2GO user might hate you for this change ;-) and me too ;-)

Imagine: If my mower goes from front to back area, it will make at least 3 WLAN roamings as well as a couple of 2.4/5GHz changes. I didn't validated, but wouldn't wonder if a couple of those might also trigger an NTRIP-Client respawn. In addition my NTRIP provider often had a 15-30 minute outage in past. In each of those issues, my mower would stop somewhere and drained out his batteries with your settings :-/

So for normal non-RTK2GO ppl, the previous values for delay, respawn, and max tries have been pretty reasonable (in my point of view)

In addition: Didn't checked fully now, but looks like config template also need to be modified (now). Please see /mower_config.schema.json

Or what do you think?

EtheriVR commented 1 month ago

Hi EtheriVR,

yes, it is configurable now, but as far as I see, with your preferred settings.

I worry all non-RTK2GO user might hate you for this change ;-) and me too ;-)

Imagine: If my mower goes from front to back area, it will make at least 3 WLAN roamings as well as a couple of 2.4/5GHz changes. I didn't validated, but wouldn't wonder if a couple of those might also trigger an NTRIP-Client respawn. In addition my NTRIP provider often had a 15-30 minute outage in past. In each of those issues, my mower would stop somewhere and drained out his batteries with your settings :-/

So for normal non-RTK2GO ppl, the previous values for delay, respawn, and max tries have been pretty reasonable (in my point of view)

In addition: Didn't checked fully now, but looks like config template also need to be modified (now). Please see /mower_config.schema.json

Or what do you think?

I think if the default gets people banned, it shouldn't be the default, and people who choose to build base station setups could also very easily configure this to not be an issue.

In addition, if your base station goes down for 30 minutes a re-spawn delay of 10 seconds for the container won't make much of a difference, and the max reconnects can be configured to be as high as you want it :)

Apehaenger commented 1 month ago

I understand your thoughts, but RTK2GO is an external service with very strict restrictions. As far as I know RTK2GO is not a default setting and in my impression most ppl have either their own base station or use a common public provider of their area (which do not have this strict restrictions).

I do have worries to break their current behavior, risk that their mower drains out somewhere and let them all change their config because RTK2GO user aren't ... ?! What shall I answer them?

But I'm probably wrong. Let's see what others say ;-)

ClemensElflein commented 1 month ago

I don't really understand the max reconnect setting. Just stopping to reconnect is never desired, right? Wouldn't it be enough to set reconnect TO to 10 sec?

If I read this correctly then it's only an issue when misconfigured anyways (http://rtk2go.com/how-to-get-your-ip-banned/). Once config is correct, current settings should not lead to a ban.

ClemensElflein commented 1 month ago

I'm with @Apehaenger on this one, let's introduce the setting and keep the defaults the same. We can mention RTK2GO in the docs explicitly.

The point about 'you can change config' goes into the other direction as well i.e. if people use RTK2GO they need to set the timeout whereas local base station just don't. Default is mostly an arbitrary selection which is suboptimal in some cases.

EtheriVR commented 1 month ago

Alright updated to the new schema and set the defaults back to what they were in the code

ClemensElflein commented 1 month ago

Thank you!