-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
gps base: add automatic base station configuration for Septentrio #3324
Conversation
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. |
6b342c0
to
6711edf
Compare
too much async either task early and proxy gui updates via invokes. |
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. |
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 I saw that there is a Below is an image where I print whether invoking is required (
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 |
so yes the SynchronizationContext does come into play. 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. |
6711edf
to
32a5837
Compare
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.
32a5837
to
f360b58
Compare
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.
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. |
@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? |
Thank you for the previous reviews and for accepting this 🙂 Looking forward to see this in the next release. |
This adds a checkbox to enable automatic base station setup for Septentrio receivers.