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 "arrivalCircle" to "nextPoint" #628

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

panaaj
Copy link
Member

@panaaj panaaj commented Oct 23, 2021

Addition of arrivalCircle to nextPoint to allow an arrival alarm to be implemented.

Copy link
Member

@tkurki tkurki left a comment

Choose a reason for hiding this comment

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

I think it would be useful to add this to https://github.com/SignalK/specification/blob/master/test/data/full-valid/course-full_tree.json#L48

Oh I just noticed that the hrefs in that doc put resources under a vessel...

@panaaj
Copy link
Member Author

panaaj commented Oct 31, 2021

Added tests for nextPoint.arrivalCircle.
I have noticed two things in the test data that don't align with the text in the proposed CourseAPI.

  1. href test value is under vessels ("href": "/vessels/vessels.urn:mrn:imo:mmsi:230099999/resources/waypoints/...")
    when in practice resources is at the root of ./signalk/v1/api.
  2. arrivalCircleEntered notification is defined as notifications.arrivalCircleEntered where the proposed notification path in the Course API PR is notifications.navigation.course.arrivalCircleEntered

Should these be updated in the test file?

@tkurki
Copy link
Member

tkurki commented Nov 3, 2021

I'll merge this, submit a new PR for fixes?

@tkurki
Copy link
Member

tkurki commented Nov 3, 2021

Oh, please rebase. Would do it myself nicely if the branch were under this repo ;-)

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