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

Add PSMDST #188

Closed
wants to merge 1 commit into from
Closed

Add PSMDST #188

wants to merge 1 commit into from

Conversation

joabakk
Copy link
Contributor

@joabakk joabakk commented Jan 25, 2021

Same as $STALK, but official proprietary sentence for seatalk.

test/PSMDST.js Outdated
})

it('0x51 Longitude converted', () => {
const delta = new Parser().parse(longitude)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is a problem with the existing Seatalk support. I assume all clients work with the path in spec: navigation.position and an object value. This will not be treated as a position, with two separate messages with a non spec path.

Do you know how the data stream works in the real world? Could the parser keep the latest value of one or the other and trigger emit on the other, making sure that the value of the other field is not too old?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of the ALK.js code. The actual code seems to be ok from #156, but the tests may not have followed?

Copy link
Member

Choose a reason for hiding this comment

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

tests/ALK.js different https://github.com/SignalK/nmea0183-signalk/blob/master/test/ALK.js#L110-L121

That works, but the position test is not the greatest, because it relies on the tests running in order - they are not independent. All of this is actually just one test https://github.com/SignalK/nmea0183-signalk/blob/master/test/ALK.js#L108-L121 and should be structured as one.

If the tests for ALK and PSMDST are the same, except for the inputs, it would be great to restructure them so that there is only one copy of the tests, running for two sets of test data.

I tried to fix the Gitub test action to run on PRs, could you please rebase on master so that we can see that tests run.


module.exports = function(input) {
const { id, sentence, parts, tags } = input
const key = '0x' + (parts[0]).toUpperCase()
Copy link
Member

Choose a reason for hiding this comment

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

Is toUpperCase really needed?

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 don't think so, but if some DIY seatalk talker would emit a 0x9c datagram, it would fail. To my knowledge, official seatalk is always uppercase

Same as $STALK, but official proprietary sentence
@joabakk joabakk mentioned this pull request Feb 2, 2021
@joabakk
Copy link
Contributor Author

joabakk commented Feb 2, 2021

Messed up repo, closing and re-opening #189

@joabakk joabakk closed this Feb 2, 2021
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.

2 participants