Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NTRIP client getting people ip banned #107

Merged
merged 6 commits into from
Jul 7, 2024

Conversation

EtheriVR
Copy link
Contributor

@EtheriVR EtheriVR commented Jun 4, 2024

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
Copy link

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
Copy link
Owner

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
Copy link

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

@EtheriVR
Copy link
Contributor Author

EtheriVR commented Jun 5, 2024

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
Copy link

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
Copy link

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

@EtheriVR
Copy link
Contributor Author

EtheriVR commented Jun 9, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

EtheriVR commented Jul 7, 2024

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
Copy link
Collaborator

Apehaenger commented Jul 7, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

EtheriVR commented Jul 7, 2024

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

@ClemensElflein
Copy link
Owner

Thank you!

@ClemensElflein ClemensElflein merged commit 7e73539 into ClemensElflein:main Jul 7, 2024
ClemensElflein added a commit that referenced this pull request Jul 9, 2024
ClemensElflein pushed a commit that referenced this pull request Jul 9, 2024
ClemensElflein added a commit that referenced this pull request Jul 9, 2024
Co-authored-by: Clemens Elflein <clemens.elflein@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants