feat(streams): native canboat sources for iKonvert, IPG100, SocketCAN, YDWG-02, NavLink2 and W2K-1 - #2909
Open
dirkwa wants to merge 4 commits into
Open
feat(streams): native canboat sources for iKonvert, IPG100, SocketCAN, YDWG-02, NavLink2 and W2K-1#2909dirkwa wants to merge 4 commits into
dirkwa wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe administration UI adds analyzer-backed NMEA 2000 sources and source-specific fields. Native stream handling validates provider values, escapes Maretron IPG passwords, uses bidirectional ChangesNMEA 2000 provider support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant NativeNMEA2000
participant ExecutePipe
participant socketcan-serial
participant CANBus
AdminUI->>NativeNMEA2000: submit NMEA 2000 source settings
NativeNMEA2000->>NativeNMEA2000: validate values and escape password
NativeNMEA2000->>ExecutePipe: configure child-process piping
ExecutePipe->>socketcan-serial: start transport when enabled
socketcan-serial->>CANBus: transmit and receive CAN frames
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cr-review-host-provided-modules.txt (1)
1-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove both generated review artifacts.
These files contain transient CodeRabbit logs instead of repository content. Delete both files before merge.
cr-review-host-provided-modules.txt#L1-L74: delete the generated review transcript.cr-review-ignore-test-sources-cache.txt#L1-L64: delete the generated review transcript and its stale artifact reference.SignalK’s contribution guide favors focused changes and separate pull requests for unrelated work. (github.com)
As per path instructions, remove leftover crumbs from intermediate commits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cr-review-host-provided-modules.txt` around lines 1 - 74, Delete the generated review transcript cr-review-host-provided-modules.txt (lines 1-74) and the stale artifact cr-review-ignore-test-sources-cache.txt (lines 1-64); neither file should remain in the repository.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx`:
- Around line 1749-1753: Update the ikonvert-canboatjs handling in the stream
implementation to preserve the configured baud rate by using subOptions.baudrate
when provided, falling back to 230400 only when it is absent. Keep the
BaudRateInputCanboat rendering in BasicProvider unchanged.
- Around line 1881-1882: Update the provider description rendering near the
maretron protocol condition to distinguish maretron-ipg from
maretron-ipg-canboatjs. Ensure maretron-ipg describes the native maretron-ipg
command, while preserving the existing canboatjs description for
maretron-ipg-canboatjs, or use wording accurate for both transports.
- Around line 974-977: Define named protocol baud-rate constants in
packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx and use them
in BaudRateInputCanboat instead of raw literals. Also define named native
transport baud-rate constants in packages/streams/src/simple.ts and use those
constants when constructing the commands in the affected 428-447 range.
- Around line 1768-1772: Update the provider form condition around HostInput and
PortInput so native ydwg02-udp renders only PortInput, removing the unused
HostInput while preserving HostInput for ydwg02-udp-canboatjs; do not add host
persistence unless the native command in simple.ts is changed to consume it.
In `@packages/streams/src/simple.ts`:
- Around line 441-443: Update the mfgCode condition in the command construction
around subOptions.mfgCode to check whether the value is absent rather than
relying on truthiness, so numeric 0 still produces the -m 0 argument while
undefined or null continues to omit it.
---
Outside diff comments:
In `@cr-review-host-provided-modules.txt`:
- Around line 1-74: Delete the generated review transcript
cr-review-host-provided-modules.txt (lines 1-74) and the stale artifact
cr-review-ignore-test-sources-cache.txt (lines 1-64); neither file should remain
in the repository.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc254a25-c944-4833-9760-e7fa3f20ca17
📒 Files selected for processing (4)
cr-review-host-provided-modules.txtcr-review-ignore-test-sources-cache.txtpackages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsxpackages/streams/src/simple.ts
Comment on lines
+974
to
+977
| const defaultBaudrate = | ||
| value.type === 'ikonvert-canboatjs' || value.type === 'ikonvert' | ||
| ? 230400 | ||
| : 115200 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Name the protocol baud-rate defaults.
Raw baud-rate literals hide protocol configuration and make UI and runtime defaults harder to keep aligned.
packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx#L974-L977: define named baud-rate constants and use them inBaudRateInputCanboat.packages/streams/src/simple.ts#L428-L447: define named native transport baud-rate constants and use them in command construction.
📍 Affects 2 files
packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx#L974-L977(this comment)packages/streams/src/simple.ts#L428-L447
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx` around
lines 974 - 977, Define named protocol baud-rate constants in
packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsx and use them
in BaudRateInputCanboat instead of raw literals. Also define named native
transport baud-rate constants in packages/streams/src/simple.ts and use those
constants when constructing the commands in the affected 428-447 range.
Source: Coding guidelines
Contributor
Author
There was a problem hiding this comment.
@coderabbitai Skipped: the baud-rate literals sit in two files with different lifetimes (a React input default and the native command construction), and hoisting them into shared constants would mean a new cross-package export for two values. The UI change above removes the cases where the two could disagree.
Contributor
There was a problem hiding this comment.
@dirkwa, acknowledged. The UI and native command paths have separate responsibilities. A shared cross-package constant would add coupling without sufficient benefit. The updated iKonvert condition keeps the relevant UI defaults aligned.
You are interacting with an AI system.
added 3 commits
August 6, 2026 15:33
…directional SocketCAN The native (analyzer) NMEA 2000 path only offered Actisense NGT-1 and a receive-only candump pipeline, while canboat ships purpose-built bridge tools for more gateways. All of them speak the same canboat CSV contract on stdout/stdin, so each new subtype is one Execute command: - maretron-ipg: Maretron IPG100 over TCP (default port 6543, optional password), bidirectional - ikonvert: Digital Yacht iKonvert over serial, bidirectional - canbus now runs canboat's socketcan-serial instead of candump | candump2analyzer: same decode path, but it claims an address and accepts outbound PGNs on stdin, honoring the same uniqueNumber/mfgCode settings as the canboatjs SocketCAN option. The old pipeline was receive-only and, having been gated behind a missing analyzer binary, had no working installations to preserve. The admin UI options are gated on hasAnalyzer like the existing native entries and reuse the field sets of their canboatjs counterparts.
The native N2K path covered NGT-1, SocketCAN, iKonvert-serial and the Maretron IPG100; the network gateways stayed canboatjs-only. canboat's interface command now bridges them (canboat/canboat#807), so each is one spawn: - ydwg02 / ydwg02-udp: Yacht Devices RAW over TCP (default 1457) or receive-only UDP, fast-packets reassembled and fragmented in the bridge - navlink2: the iKonvert protocol over TCP (default 6001) - w2k-1-ascii: Actisense N2K ASCII over TCP (default 60002) Admin UI options are gated on hasAnalyzer like the existing native entries and reuse their canboatjs counterparts' host/port field sets.
…ay commands Execute runs through a shell: hosts, devices and interfaces are now checked against the character set their legitimate values need, and ports, baud rates and NAME fields must be finite numbers — a malformed value fails the provider loudly instead of reaching sh -c (or turning into '-u NaN'). The receive-only ydwg02-udp bridge also stops wiring stdin, so the admin UI no longer reports transmit activity on a connection that cannot transmit.
dirkwa
force-pushed
the
pr-native-gateways
branch
from
August 6, 2026 03:34
c2f325e to
07771ca
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/streams/src/simple.ts`:
- Around line 450-455: Update the maretron-ipg command construction in the
password-handling branch so credentials are sent only over authenticated
encrypted transport rather than plain tcp://. Use the native process’s supported
TLS/secure-connection option, or reject this configuration before launching when
encryption cannot be guaranteed; do not pass the password over an unencrypted
connection.
- Around line 459-462: Update the ydwg02-udp branch and its Execute.pipe
integration so this receive-only provider explicitly disables stdin wiring
instead of relying on undefined toChildProcess and the default event fallback.
Ensure plugin-emitted toChildProcess events cannot write to the child process,
while preserving normal stdin behavior for other provider types.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67a2d876-df35-4197-8f7d-8e54fcb654d7
📒 Files selected for processing (2)
packages/server-admin-ui/src/views/ServerConfig/BasicProvider.tsxpackages/streams/src/simple.ts
- socketcan-serial: check for an absent uniqueNumber/mfgCode instead of a falsy one, so the valid value 0 (the form offers it as 'Internal') reaches the child process. - Execute: 'toChildProcess: false' disables the stdin wiring outright. Leaving it undefined only subscribed to the default event, so a receive-only provider still wrote to the child on any plugin's outbound PGN; ydwg02-udp now sets it. - The admin UI stops offering controls the server ignores: the baud rate for the canboatjs iKonvert and YDWG-02 USB elements (both pin their protocol's rate), and the host for the native ydwg02-udp bridge (it listens on a port and never dials a host). - The Maretron description states that the password crosses the wire in the clear during the gateway's handshake; the protocol offers no encrypted alternative, so this is disclosure rather than a fix.
Contributor
Author
|
ready for human review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the existing native-canboat connection pattern (
ngt-1/canbusvia theanalyzertoolchain) to the remaining common gateways: Digital Yacht iKonvert, Maretron IPG100 (including its session password), bidirectional SocketCAN via canboat'ssocketcan-serial(address claim + outbound PGNs, which a candump pipeline cannot do), Yacht Devices YDWG-02 (TCP and receive-only UDP), NavLink2, and Actisense W2K-1 N2K ASCII. All hasAnalyzer-gated in the admin UI, exactly like the existing native options — nothing changes for installations without the canboat tools.The last commit hardens the whole native block (pre-existing pattern included): every provider-config value interpolated into the shell command is validated first — character-set checks for hosts/devices/interfaces, finite-number checks for ports, baud rates and NAME fields — and the receive-only ydwg02-udp bridge no longer wires stdin, so the UI stops reporting transmit activity it cannot have.
Companion of #2908 (analyzer camelCase normalization); independent to review and merge.
Tested: streams typecheck and unit tests; every gateway type has been running in a staging image — IPG100 verified against live hardware on a real N2K network.
Summary
This PR adds native NMEA 2000 analyzer sources for iKonvert, Maretron IPG100, bidirectional SocketCAN, YDWG-02, NavLink2, and Actisense W2K-1 ASCII.
hasAnalyzer.