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

feature: Add NMEA stream handling to Course API. #1750

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

Conversation

panaaj
Copy link
Member

@panaaj panaaj commented Jun 8, 2024

Provide the ability for the Course API to set the destination based on the data received from NMEA connections. This allows application developers to use V2 Course API with course data coming from NMEA 0183 and NMEA 2000 sources.

Delta messages with a source.type of NMEA0183 or NMEA2000 containing path values for

  • navigation.courseRhumbline.nextPoint.position OR
  • navigation.courseGreatCircle.nextPoint.position

will trigger invoke the course setDestination() method.

Source priorities:

The following priorities are applied when determining whether a destination will be set from an NMEA delta:

  1. Destination requests via Course API endpoints have the highest priority and will ALWAYS be actioned!
  2. If a destination is set, only updates from the same source are actioned (unless it is an API request)
  3. If no destination is currently set, the first NMEA delta containing nextPoint.position data will be used
  4. If apiOnly mode is enabled then NMEA sources are ignored.

Approach:

A subscription has been added to the Course API for the paths navigation.courseRhumbline.nextPoint.position OR navigation.courseGreatCircle.nextPoint.position to listen for deltas generated by NMEA data connections and set / clear the destination using API methods.

Note: The destination set by an NMEA source WILL NOT be cleared when the NMEA source sentence contains no data due to a nmea0183-tosignalk plugin processing issue (See SignalK/nmea0183-signalk#255)

In the default mode (apiOnly=false)
Clearing a destination using an API request, when an NMEA source still has an active destination, will see the destination set to the value provided by the NMEA data.

Enabling apiOnly mode using HTTP POST ./course/_config/apiOnly will:

  1. Clear the NMEA sourced destination
  2. Only process requests via HTTP API
  3. Cause delta messages from NMEA sources to be ignored

Disabling apiOnly mode (default) using HTTP DELETE ./course/_config/apiOnly will:

  1. Enable processing of both HTTP API request and delta messages from NMEA sources

Set / clear destination from NMEA  stream data.
@panaaj panaaj added the feature label Jun 8, 2024
@panaaj panaaj requested a review from tkurki June 8, 2024 02:28
@panaaj panaaj changed the title Feature: Add NMEA stream handling to Course API. feature: Add NMEA stream handling to Course API. Jun 8, 2024
Fixes issue of corrupted nextPoint  value when emitting v1 course delta.
Setting this param causes the stream input to be ignored until the endpoint is called again without specifying `force`.
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.

Did I understand the idea here correctly: we don't touch the 0183 and N2K parsers and instead make course API act on the sk messages they produce?

docs/src/develop/rest-api/course_api.md Outdated Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
src/api/course/openApi.json Outdated Show resolved Hide resolved
src/api/course/openApi.json Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
src/api/course/index.ts Outdated Show resolved Hide resolved
@panaaj
Copy link
Member Author

panaaj commented Jun 17, 2024

Did I understand the idea here correctly: we don't touch the 0183 and N2K parsers and instead make course API act on the sk messages they produce?

Short answer is yes.

After considering a number of approaches I landed on this approach for the following reasons:

  1. NMEA PGNS / Sentences already parsed
  2. New PGNs added to N2K parser are immediately supported
  3. Passively "taps" into a single input stream of data (which includes all input sources)
  4. Provides option to "filter / change" the incoming message (if required)
  5. Does not introduce breaking changes for applications and plugins (as input stream remains unchanged)
  6. Provides a workable MVP which can facilitate a managed transition to an alternative approach.

src/api/course/openApi.json Outdated Show resolved Hide resolved
@panaaj panaaj requested a review from tkurki June 22, 2024 05:18
@panaaj
Copy link
Member Author

panaaj commented Sep 4, 2024

@tkurki is there anything further required here?

@panaaj
Copy link
Member Author

panaaj commented Nov 2, 2024

@tkurki Have updated this to provide end points to store settings to enable the provisioning of a control in the server admin-ui.

I have not included any changes to the admin-ui in this PR.
Did you want chnages to the server admin-ui in a separate PR?

@tkurki
Copy link
Member

tkurki commented Nov 10, 2024

I gave this a go with https://github.com/SignalK/signalk-server/blob/master/samples/gofree-merrimac.log that has BWC and RMB. Nextpoint shows up nicely in Freeboard.

Just one thing that I noticed: should the source for course data be courseApi or should it be the original source of the nextpoint data?

image

@tkurki
Copy link
Member

tkurki commented Nov 10, 2024

Have you been able to test this out in real world scenarios/data?

@tkurki
Copy link
Member

tkurki commented Nov 10, 2024

I guess Swagger provides the minimal UI for this, so we can merge without a UI. Do you have ideas as to where in the Admin UI this setting would go?

@tkurki
Copy link
Member

tkurki commented Nov 10, 2024

The docs now refer only to 0183, but PGN 129284 also provides nextPoint - update the docs to cater for this?

I updated the PR description with a sentence as to motivation for this feature - what it allows.

@panaaj
Copy link
Member Author

panaaj commented Nov 11, 2024

Just one thing that I noticed: should the source for course data be courseApi or should it be the original source of the nextpoint data?

Probably should be the original source. I can make that change.

@panaaj
Copy link
Member Author

panaaj commented Nov 11, 2024

Have you been able to test this out in real world scenarios/data?

I have tried it with as many source samples I could get and with my NMEA simulator tool.
Because it is listening to the signal K paths populated by the nmea0183 and n2k to signalk plugins it is reliant on their mappings, which I would assume are well proven.

@panaaj
Copy link
Member Author

panaaj commented Nov 11, 2024

I guess Swagger provides the minimal UI for this, so we can merge without a UI. Do you have ideas as to where in the Admin UI this setting would go?

I think it aligns with being located in Server Settings with the other settings saved to the settings.json file, as it will also require a server restart for the settings file to be re-read.

Something like.....
image

If the admin-UI called the api endpoint to change mode then it would not require a server re-start. In this case it might want to be located in another section.

@panaaj
Copy link
Member Author

panaaj commented Nov 11, 2024

Have you been able to test this out in real world scenarios/data?

Only with the lmited devices I have and the data sources I can connect to. As part of that testing I raised SignalK/nmea0183-signalk#255 as nextPosition was not getting cleared when NMEA sentence contained no data.

@panaaj
Copy link
Member Author

panaaj commented Nov 11, 2024

The docs now refer only to 0183, but PGN 129284 also provides nextPoint - update the docs to cater for this?

I updated the docs to reflect that the Course API looks at the the following paths populated by the nmea0183-to-signalk and n2k-to-signalk plugins...

  • navigation.courseRhumbline.nextPoint.position
  • navigation.courseGreatCircle.nextPoint.position

Sentences and PGNs can then continued to be managed in these plugins as required.

@panaaj
Copy link
Member Author

panaaj commented Nov 12, 2024

Just one thing that I noticed: should the source for course data be courseApi or should it be the original source of the nextpoint data?

Probably should be the original source. I can make that change.

Thinking about it a little further, I did originally consider this but it adds some source management overhead so as not to leave a litter of course.nextPoint delta paths with conflicting values when there are a number of sources. Even managed correctly it will leave a litter of course.nextPoint delta paths with null values for the sources that have been used.

If the source should reflect the NMEA source, should the value be a combination of NMEA source id and courseApi to reflect that the Course API used the NMEA source?
Noting that the navigation.courseGreatCircle.nextPoint.position value will reflect the NMEA source id.

@tkurki
Copy link
Member

tkurki commented Nov 12, 2024

I think the lifecycle of the values is kind of a separate concern - the same thing happens for all values that have had a value and then turn to null. I think there is more value in having the real source of the course destination available.

I think you're thinking too much into the role of the logic in this PR. To me there's input from a source (0183, N2K, api calls) that is reflected in course API values. As an API user I might want to know where the value that I am seeing came from, not how the server works internally.

I would also imagine that in most cases where this integration kicks in the course is primarily managed outside sk server, in the environment where the nmea data is coming from, so littering should be less of a problem.

I started working on a generic timeout mechanism, maybe that should be implemented so that data is actually removed some day, not just cleared by sendin null 🤔 .

@panaaj
Copy link
Member Author

panaaj commented Nov 12, 2024

Just so I'm clear we are only talking about the source of course.nextPoint not all the paths set by Course API.

@panaaj
Copy link
Member Author

panaaj commented Nov 13, 2024

I have made an update that sets the source of the course.nextPoint delta to source id.
This seems to have broken the tests now that this delta is sent indepentently from the rest.

Add & use type discriminators for narrowing Update
down to having values or meta. Avoids meta is more
typescriptish.
Separate cases for
(1) clear destination
(2) update destination

Less exit points for clarity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants