-
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
Improvements to Altitude Angel integration #3147
Conversation
…precision on altitude.
@meee1 this is ready for you to review. If you have any questions, just ask. I've kept 99.9% of the changes to the AltitudeAngelWings assemblies, with one minor exception - removing app.config settings (as we're no longer using them). |
Hi Rupert, Thanks for the update, I merged and tested your PR (with master 142d367) and seems to be now working as expected with a basic user account login. I did however find one bug on Windows 11 where it was throwing an exception during Mission planner shutdown. I found the issue was in the MessagesService class and was able to fix as follows: In the MessagesService constructor where Messages.Subscribe() is called, this call returns an IDisposable which can be saved as an instance variable and then disposed immediately before calling Messages?.Dispose() In my case this prevents RemoveMessage() still being called after Messages has been disposed which was the cause of the exception (see stack trace at end). That said, according to ReplaySubject(T).Dispose Method (System.Reactive.Subjects) | Microsoft Learn this should be done anyway so it might be some sort of race condition so perhaps there is a more reliable solution than this? P.S I can post my modified code if necessary but I think it should be clear based on above description although perhaps you will not be able to reproduce anyway since from the github checks it looks like you might be on OSX? Cheers, casrya -- |
Thanks @casrya, good spot. You're correct this is to do with disposal timing, but actually |
Hi Rupert, Tested here and all good, the Cheers, casrya |
Thanks @casrya , that's good to hear. Anything else you spot or improvements you'd like to see, please feel free to raise an issue and @ me or just get in touch. |
@meee1 thanks for merging the fix. I've merged into here and now the builds are passing again. |
Hi @meee1 any feedback on this? Really keen to get this merged as we have a few other things lined up behind this. |
ok reviewed and merged |
Many thanks @meee1. |
Updates with recent improvements to Altitude Angel services to enable them in Mission Planner. This includes: