Skip to content

Add "arrivalCircle" to "nextPoint"#628

Closed
panaaj wants to merge 3 commits into
SignalK:masterfrom
panaaj:course-nextpoint-arrivalCircle
Closed

Add "arrivalCircle" to "nextPoint"#628
panaaj wants to merge 3 commits into
SignalK:masterfrom
panaaj:course-nextpoint-arrivalCircle

Conversation

@panaaj

@panaaj panaaj commented Oct 23, 2021

Copy link
Copy Markdown
Member

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

@tkurki tkurki left a comment

Copy link
Copy Markdown
Member

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

panaaj commented Oct 31, 2021

Copy link
Copy Markdown
Member Author

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

tkurki commented Nov 3, 2021

Copy link
Copy Markdown
Member

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

@tkurki

tkurki commented Nov 3, 2021

Copy link
Copy Markdown
Member

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

@panaaj panaaj closed this by deleting the head repository Jun 19, 2026
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