meetjestad / mjs_firmware

8 stars 4 forks source link

Improve GPS stability #16

Closed ketilmo closed 5 years ago

ketilmo commented 5 years ago

I believe the GPS acquisition problems are related to the deep sleep mode the stations enter after a transmission. I have modified the code to reinitialize the GPS sensor before every position acquisition. The change has been deployed to my station, which was losing periodically (every few days, typically). After the modification, this station has been running for more than three weeks without any GPS loss.

ketilmo commented 5 years ago

Hi @matthijskooijman,

I have now tested this firmware modification on three separate devices that were experiencing GPS issues. After the firmware update, they have been running 100% stable. Here are the stations in question: 360, 365 and 367. See the behavior before and after last reset for each device. The pattern is pretty clear.

dianawi commented 5 years ago

@matthijskooijman can we merge this? In Bergen we have a bit more gps trouble than in NL, maybe because of the mountains.

ketilmo commented 5 years ago

@matthijskooijman @dianawi The flashed stations still run very stable – the issues appear to have disappeared completely. Station 365 has done more than 6500 transmission without a single missing GPS position. (Note that this specific station also worked the last few weeks before the update to this firmware version, as I tested falling back to hard coded coordinates. But if you go some more weeks back in its history, you will se it experiencing GPS problems.)

ketilmo commented 5 years ago

Hi @matthijskooijman,

Thanks for your input. When I started trying to resolve the GPS issue, I followed a broad spectrum approach. A basically did multiple changes at once to see if that improved the situation, and it did. Ideally, we should try reverting change by change now. The problem is that it can take days for the bug to surface, making this a hard debug.

The reason I made the SoftwareSerial object local, was to ensure that the GPS initialises a fresh on every GPS acquisition attempt. Conserving memory between the daily attempts sounds like a nice side effect. :)

In that line - could the gps_data variable be made a local variable now?

The gps_data variable is still used in the dumpData method, in addition to the setPosition method. Making it local would hence require some more code changes.

I will answer the rest of your comments inline.

matthijskooijman commented 5 years ago

Thanks for your replies and amendments, you make excellent points :-)

Have you been running with the amended code from this PR since then? Does it still run stable? If so, then we can at least rule out the increased GPS timeout as a cause for improvements, which would be good.

One thing that I'm still wondering is whether lat24/lon24 should be added as global variables. If we can't make gps_data local, as you correctly argue, then this change adds memory rather than saving it, which I'm reluctant to do. If we do not add these variables and leave the encoding of the data for later, then we must make sure that gps_data is reset to be invalid, of course. I suspect this already happens in the GPS read loop if gps.available() ever returns once (since this overwrites gps_data), but there might be a case where gps.available() never returns true (but that indicates a broken GPS, I think). We could fix this be resetting gps_data.

Having said that - it's probably good to merge the code as it is now (if you can confirm it still runs good) and then continue tweaking and testing after that. Howe does that sound?

ketilmo commented 5 years ago

Hi @matthijskooijman,

Thanks for your kind reply, it's appreciated. I have not yet flashed the units with the amended code, as I wanted to get your feedback first. When it comes to the GPS timeout value, increasing it was the first thing I did when debugging initially. I tried several different values, even doubling to six minutes, but this had no effect on the stability. So I think it's safe to say that the GPS timeout value is not a factor in the specific fix problem cases that I've been investigating.

I think your suggestion for next steps is good. If we merge this fix, we can then consider iterating further. If you want, I'll flash my station with the current suggestion to verify that it still works as expected – let me know if you want me to do this. The two other stations is a bit harder for me to get to in the short term as they are geographically dispersed. The good news is that all three of them has (still) been running completely stable since the fix was deployed. There have not been a single GPS hitch on any of them.

There will be a MJS gathering in Bergen on June 11, where people who have problems with their stations will bring them in for debugging. If we are able to release the fix before that, it would be perfect timing.

Have a nice day!

Kind regards, Ketil

matthijskooijman commented 5 years ago

I think your suggestion for next steps is good. If we merge this fix, we can then consider iterating further. If you want, I'll flash my station with the current suggestion to verify that it still works as expected – let me know if you want me to do this.

Yes, please :-)

ketilmo commented 5 years ago

@matthijskooijman: I have now flashed station 365 with the newest firmware amendments. I will run it for a few days, and report back when I've verified that everything looks good.

ketilmo commented 5 years ago

@matthijskooijman The station has been running stable since the update 6 days ago, so I think we're all set now.

ketilmo commented 5 years ago

@matthijskooijman Still stable after 12 days.

matthijskooijman commented 5 years ago

I've taken your changes and redivided them into two separate commits (one for each logical change) and removed some trailing whitespace as well (no other changes to the code). With that, I think this is ready to merge.

ketilmo commented 5 years ago

Great! Thanks a lot, @matthijskooijman. I will start rolling this out today, then. :)