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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

flyingthingsintothings
Copy link

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

@flyingthingsintothings flyingthingsintothings marked this pull request as draft April 8, 2024 12:52
@flyingthingsintothings
Copy link
Author

flyingthingsintothings commented Apr 8, 2024

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

meee1 commented Apr 10, 2024

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

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

flyingthingsintothings commented Apr 10, 2024

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

meee1 commented Apr 11, 2024

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 flyingthingsintothings marked this pull request as ready for review April 11, 2024 13:45
Make the automatic base setup more generic so it can also be used with
other receivers. Add configuration options for Septentrio receivers.
Use `beginInvoke()` inside asynchronous functions to update the UI from
the UI thread.
When the user quickly clicked the "connect" button twice, the task to
connect to the receiver would be executed twice, causing problems.
Disable the button until the connection process is finished.
@flyingthingsintothings
Copy link
Author

flyingthingsintothings commented Apr 11, 2024

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.

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.

None yet

2 participants