ArduPilot / MissionPlanner

Mission Planner Ground Control Station for ArduPilot (c# .net)
http://ardupilot.org/planner/
GNU General Public License v3.0
1.68k stars 2.33k forks source link

gps base: add automatic base station configuration for Septentrio #3324

Closed flyingthingsintothings closed 2 weeks ago

flyingthingsintothings commented 2 months ago

This adds a checkbox to enable automatic base station setup for Septentrio receivers.

flyingthingsintothings commented 2 months ago

I added a checkbox to the ConfigSerialInjectGPS screen that allows Septentrio receivers to be automatically set up. I'm not the most familiar with C#, .NET and winforms so some things may need to be changed.

I also changed the click handler for the "connect" button to be asynchronous instead as it doesn't block the entire UI when using other async methods.

meee1 commented 2 months ago

too much async the issue is that you will likely see cross thread gui calls because of the gui updates from the async jobs. causing problems.

either task early and proxy gui updates via invokes. or no async, or async only where there is no gui changes

flyingthingsintothings commented 2 months ago

Oh OK. I haven't written enough C# winforms code to know whether this would cause issues. I was trying to avoid blocking the UI thread as the current automatic base setup does that which prevents any interaction with the UI for some seconds. I'll look a bit further into the .NET with winforms to see how this can be done.

flyingthingsintothings commented 2 months ago

I read about the asynchronous model in .NET with winforms a bit and I don't fully understand what would cause the cross-thread UI calls. From what I understand, each await captures the current SynchronizationContext (the UI one in this case) and resumes in that one when it continues executing. I must be missing something but I don't know what.

I saw that there is a ConfigureAsync(bool) method that does not ensure a Task captures its SynchronizationContext, but I don't use it anywhere and I think the UI framework calls the button click handler without that configuration as well. Is there a reason why there would be cross-thread UI updates?

Below is an image where I print whether invoking is required (Control.InvokeRequired) both right after the BUT_connect_Click() handler as well as right before the handler is finished (at the end of DoConnect()). I tested this with both a Debug and Release build and the results are the same.

image

or no async

I wanted to use an approach that doesn't block the UI thread. The current automatic base setup does this and it doesn't create the best user experience. From what I can tell there are two possible approaches: real threads and async tasks. Since async tasks were made for this purpose (nicely handling blocking code with understandable control flow), I chose that approach.

Sources

meee1 commented 2 months ago

so yes the SynchronizationContext does come into play. you start in the UI thread. you run async task. this may or may not be on the ui thread. once the task has completed it will return to the UI thread. but the code inside the await. could be on any thread. while your test shows it actually didn't change threads. that's not a 100% guarantee it wont happen in the future. you can begininvoke to queue a update that will ensure the correct SynchronizationContext /UI thread.

the moral is, no GUI stuff on any thread other than the UI thread. this means setting properties on controls that could cause a refresh on that control.

ie with the current code i could bash the connect button multiple times, and it would cause issues with the serial port being in use etc.

flyingthingsintothings commented 2 months ago

I made the requested changes and finished the feature. I tested this extensively by setting different combinations of options, going out of the view and back with a connection active, using different Septentrio receivers connected through different ports. It all seems to work well. I also tested the u-blox code to make sure it still fully worked, which seems to be the case. I tried to document all the new functions and added tooltips where it made sense.

I'm sorry that it turns out to be such a big PR, but the feature required some refactoring in the UI to offer users the choice between different receivers. Most of the changed lines are therefore in the generated UI files. I will make sure to update the official documentation with the necessary changes as well.

flyingthingsintothings commented 2 weeks ago

@meee1 Is there anything else I can add or change to this pull request or would it be ready for a review? Is there maybe a coordination call where this pull request could be discussed and prepared to be merged?

flyingthingsintothings commented 2 weeks ago

Thank you for the previous reviews and for accepting this 🙂 Looking forward to see this in the next release.