NMEA support improvements #1939
Conversation
- Add AIS parser, including AIS target manager - Add messages for engine surveillance - Improve performance of Geoid calculations - Add NMEA Udp server - Add NMEA TCP client - Fix bugs - Improve simulation
src/devices/Nmea0183/samples/NmeaSimulator/NmeaSimulator.csproj
Outdated
Show resolved
Hide resolved
Test coverage for Nmea0183 namespace is now ~80%
|
/azp run dotnet.iot |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run dotnet.iot |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Need to figure out why
Therefore only run this test with .NET 6.0
| /// <returns>True if a valid position is returned</returns> | ||
| public bool TryGetCurrentPosition(out GeographicPosition? position, bool extrapolate, out Angle track, out Speed sog, out Angle? heading) | ||
| public bool TryGetCurrentPosition( |
There was a problem hiding this comment.
I might be missing something but why is this function in the SentenceCache? I'd expect this code to only do caching without any extra logic
There was a problem hiding this comment.
You are right, I separated these methods to a separate class. To much logic in the same class is poor design.
| /// <summary> | ||
| /// A helper DTO to transfer engine data in one blob | ||
| /// </summary> | ||
| [Serializable] |
There was a problem hiding this comment.
Because DTOs should generally have this attribute.
| /// This class helps finding a free network port for a test. | ||
| /// Could be moved to the real library, but with what name? | ||
| /// </summary> | ||
| internal class NetworkPortScanner |
There was a problem hiding this comment.
I'd really prefer we did not have to use TCP or UDP anywhere in the product and not have to test it this way
There was a problem hiding this comment.
(these kind of stuff tends to be flaky and networking code is always a bit fragile and prone to errors)
There was a problem hiding this comment.
I know, but this is the best possibility to really do an integration test on several parts at once. And it did help me find compatibility bugs on different platforms (e.g. macOS has several weird bugs in the UDP code).
There was a problem hiding this comment.
Sorry this took so long but it was really large PR 😄
Generally speaking looks good to me, there is only one blocking item left before we merge:
- ensure to add comment with attribution to original code if needed also in tests (if not needed then ignore this)
As a follow up I'd recommend to clean up this code and remove as much code as possible without losing much functionality:
- remove networking utilities - while they're useful it's better to focus on core abstraction and provide something more general purpose separately
- try to cleanup the code, generalize repeating code etc
So the test location is independent from the source location
Done.
I kind of agree. It would be good if we could separate such utilities for general use. We'll have to see how to best achieve that.
That could be more difficult. There's indeed a lot of boilerplate code here, but since the messages don't follow a general scheme but instead are individually defined, I don't see much potential there. I'm somewhat trying to achieve that with the high-level interfaces, so that (for instance) one can call |
Microsoft Reviewers: Open in CodeFlow