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

Adding UnixTimestamp + Parse message type 27 #149

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

joshdadswellijyi
Copy link
Contributor

@joshdadswellijyi joshdadswellijyi commented May 6, 2022

I have come accross the need to have the UnixTimestamp from the Nmea message so I implemented the following in order to accomplish that.

Library already read and extracted the timestamp I just needed to add it to the message records so it could be used outside the library.

This pull request also contains the parsing of message type 27 which you already had a decoder for in Ais.Net

@CLAassistant
Copy link

CLAassistant commented May 6, 2022

CLA assistant check
All committers have signed the CLA.

@joshdadswellijyi joshdadswellijyi changed the title Adding UnixTimestamp Adding UnixTimestamp + Parse message type 27 Jun 22, 2022

public interface ITimestamp
{
public long? UnixTimestamp { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unix timestamps come in two different units: seconds and milliseconds. I don't know which this is. Please could you rename it to make the units clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was simply using the field name that you had used to maintain continuity but can rename if desired?

https://github.com/ais-dotnet/Ais.Net/blob/3f0624713cd879f0234b167e9e77b899765e8843/Solutions/Ais.Net/Ais/Net/NmeaTagBlockParser.cs#L141

@@ -87,7 +93,7 @@ public void OnCompleted()
throw new NotImplementedException();
}

private void ParseMessageTypes1Through3(ReadOnlySpan<byte> asciiPayload, uint padding, int messageType)
private void ParseMessageTypes1Through3(NmeaLineParser nmeaLineParser, ReadOnlySpan<byte> asciiPayload, uint padding, int messageType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we're now passing in a line parser and the ASCII payload seemed like a code smell to me, and now has me wondering: is there a layering issue here?

The timestamp isn't really part of the AIS message, it's actually from the surrounding NMEA layer, isn't it?

Before this change, these various message types represent the information that was broadcast by the vessels' AIS systems. The timestamp is not part of that. The provenance of the timestamp is not necessarily clear. It might tell us when the ground station that picked up the radio transmission received it. But in cases where messages are relayed as part of a large-scale network that collects AIS data from numerous stations, it might just tell you when the particular equipment you're plugged into received the message.

So for that reason I'm not sure if it is really correct to attach this to the types that represent AIS messages.

If you need access to information from the NMEA layer, might it be better to design in something where we make that available, and make it clear which pieces of information came from where?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again my scope to refactor and improve general code quality is limited as I have many other commitments but would welcome PRs on my repository if you dont want to undertake this work in yours.

@HowardvanRooijen
Copy link
Contributor

@joshdadswellijyi - I've split out Ais.Net.Models to their own project, and implemented AisMessageType27 type.

That's now referenced in Ais.Net.Receiver via the nuget package.

I haven't implemented any of your Unix timestamp behaviour, because I don't believe we'd reached a conclusion as how best to implement the desired outcome.

@oskaremil
Copy link

Jumping into this discussion as I have a specific problem that partly includes the need for unix timestamps.

We are testing AIS.Net as the receiver component for live analysis of AIS data.

We do not want to miss messages if we can avoid it, so in cases of application upgrades there will be a limited period of time where we have two receivers (R1 is running, R2 is deploying, R1 will shut down when R2 starts to receive messages) that pump messages into our data stream.

There will be duplicates in our data stream and that is OK, since they will be filtered out later but to filter them out we need some sort of message uniqueness that I haven't been able to find in any of the parsed IAisMessage.

A combination of the source station id and unix timestamp in \s:2573***,c:1736335308*01\ combined with message type and MMSI would give us enough data to check uniqueness.

My proposal, to keep backwards compatibility is as simple as adding .Metadata as a subject on the receiver host where the observable is of this structure:

public class Metadata
{
    public int StationId { get; set; }
    public long UnixTimestamp { get; set; }
    public IAisMessage Message { get; set; }
}

giving users three options:

host.Messages.Subscribe(MessageAction);
host.Metadata.Subscribe(MetadataAction);
host.Sentences.Subscribe(StringAction);

@HowardvanRooijen
Copy link
Contributor

@oskaremil - that's a very interesting scenario. One of the next things I wanted to look at was decoding the AIS messages and pushing them into NATS and I had an idea about using native NATS features to de-duplicate... but I've not got any further than thinking "that would be nice".

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.

6 participants