Repository navigation
Conversation
Implements the two new features introduced by the released VISSv3.2 specification (https://github.com/COVESA/vehicle-information-service-specification/releases/tag/v3.2): - Multi-signal Get/Set: a single get/set request can now address an array of signals, potentially spanning multiple HIM trees, via a top-level "data" field instead of the single "path" field (CORE sections 5.1.1.2 and 5.2.2). Not supported over HTTP, per spec. - Core routing: new isMultiSignalRequest/handleMultiSignalRequest/ handleMultiGetRequest/handleMultiSetRequest in vissv2server.go, resolving each signal against its own tree and associating any validation error with the first failing element, per spec. - serviceMgr: new handleServiceMultiSet (handleServiceGet already supported the array-shaped response via its existing path-array unpacking). - gRPC: additive MultiGetRequest/MultiSetRequest RPCs and messages in the .proto, so existing v3.1 GetRequest/SetRequest clients and servers remain wire compatible (no changes to the existing messages). - Discovery: a new dedicated "discover" action for Signal Discovery (metadata about a signal subtree) and Forest Discovery (metadata about the set of trees the server manages), per CORE section 5.5. - Core routing: new isDiscoverRequest/handleDiscoverRequest, answered directly without going through serviceMgr, mirroring the pre-existing "metadata" filter variant. - gRPC: additive DiscoverRequest RPC and messages. - Fixed a pre-existing bug in himJsonify/viss.him parsing: the flat, non-indented block format used by viss.him is not valid YAML (duplicate top-level keys), so yaml.Unmarshal silently failed and Forest Discovery / the pre-existing HIM "metadata" filter variant would have returned no metadata. Added preprocessHimYaml to re-indent the file into valid nested YAML before parsing. - JSON schema (server/vissv2server/vissv3.0-schema.json): replaced with the VISSv3.2 schema from the specification repository, with the missing multi-signal get/set request branches added (the published spec schema only fully covers the discover action; the multi-signal request shapes were only documented in prose/examples). - gRPC (.proto + regenerated .pb.go/_grpc.pb.go): additive messages/ RPCs only; GetRequestMessage/SetRequestMessage are unchanged from v3.1, so existing gRPC clients/servers remain wire compatible. - utils/grcputils.go: JSON<->protobuf conversion functions for the new MultiGet/MultiSet/Discover messages, following the existing Get/Set/Subscribe/Unsubscribe pattern. - Data-compression cache (wsMgr/udsMgr): getDcConfig now also recognizes a top-level "data" array (not just the pre-existing "paths" filter variant) as a multi-path response for path/ts compression purposes. - Test client: testClient.go dispatches multi-signal get/set to the additive gRPC RPCs and discover to the new DiscoverRequest RPC; testRequests.json and the Javascript example commands gained multi-signal and discover examples for ws/mqtt/grpc/uds. - README.md: documents the new VISSv3.2 features. Extensive new unit tests cover the new core-routing handlers (including a live in-memory VSS tree + HIM forest fixture for Signal/Forest Discovery and multi-signal path resolution), the new serviceMgr handler, the new gRPC server methods, and the new grcputils.go conversion functions - all passing, with no regressions in the existing suite (verified against the pre-existing master baseline; the only failing tests on this Windows dev environment are pre-existing Windows-only file-permission/timing flakes present identically on master).
…hake
The CI workflow's `go test -race` run intermittently failed on this
test with:
managerhandlers_dispatch_test.go:82: forwardWsRequest returned
without pushing to backend channel
Root cause: the test raced two channels in a single select -
`clientBackendChannel` (the value forwardWsRequest sends) and `done`
(closed immediately after forwardWsRequest returns). forwardWsRequest
always sends to clientBackendChannel strictly before returning, so by
the time done is observably closed the backend channel is guaranteed
to already hold the value - but once both channels are ready at
roughly the same instant, Go's select picks between ready cases
uniformly at random. That made the test fail on the "done" arm
roughly half the time under load/race-detector timing, despite the
helper having actually done its job correctly.
Confirmed with `go test -run TestForwardWsRequest_PerformsHandshake
-race -count=500`: reproduces on master identically (pre-existing bug
in the test, unrelated to the v3.2 feature work), and no longer
reproduces after this fix (2000/2000 iterations pass).
Fix: read only clientBackendChannel in the select, with a one-second
timeout as the sole "helper never sent anything" failure signal
instead of racing it against done.
Signed-off-by: Ulf Bjorkengren <ulfbjn@gmail.com>
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
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.
Summary
Implements the two new features introduced by the released VISSv3.2 specification on a new
v3.2branch created frommaster(which implements VISSv3.1).get/setrequest can address an array of signals — potentially spanning multiple HIM trees — via a top-level"data"field instead of the single"path"field. Not supported over HTTP, per spec."discover"action for Signal Discovery (metadata about a signal subtree) and Forest Discovery (metadata about the set of trees the server manages), supplementing the pre-existing"metadata"filter variant on a Read request.Both features are supported over WebSocket, MQTT, UDS, and gRPC (multi-signal Set is not supported over HTTP, per spec; the pre-existing single-signal Get/Set/Discovery-via-metadata-filter remain unchanged and fully backward compatible over HTTP).
Details
Core routing (
server/vissv2server/vissv2server.go)isDiscoverRequest/handleDiscoverRequest: answers"discover"requests directly (Signal Discovery via the existingsynthesizeJsonTreemachinery, Forest Discovery viahimJsonify), mirroring how the pre-existing"metadata"filter variant is handled inline.isMultiSignalRequest/handleMultiSignalRequest/handleMultiGetRequest/handleMultiSetRequest: resolve each signal in the"data"array against its own HIM tree, and — per spec — associate any validation error with the first failing element. Successful multi-get requests reuse the existing array-shapedserviceMgrresponse path; multi-set forwards a resolved{"path","value"}array to a newserviceMgrhandler.viss.him's flat, non-indented block format is not valid YAML (duplicate top-level keys), soyaml.UnmarshalinhimJsonifywas silently failing (this pre-existing bug also affected the pre-existing"metadata"filter'sHIMbranch). AddedpreprocessHimYamlto re-indent the file into valid nested YAML before parsing.serviceMgr
handleServiceMultiSet, applying each resolved{"path","value"}pair via the state-storage backend (mirrorshandleServiceSet).handleServiceGetalready supported the array-shaped response needed for multi-get via its existing path-array unpacking, so no change was needed there.JSON Schema (
server/vissv2server/vissv3.0-schema.json)Replaced with the VISSv3.2 schema from the specification repository. The published spec schema fully covers the
discoveraction but — on inspection — never actually addedoneOfbranches for the multi-signal get/set request shapes (only the response/discovery shapes were added); I added the missingdata-array request branches forget/setso the schema fully validates the wire formats documented in the spec's prose/examples.gRPC (
.proto+ regenerated.pb.go/_grpc.pb.go)Additive only: new
MultiGetRequest/MultiSetRequest/DiscoverRequestRPCs and messages.GetRequestMessage/SetRequestMessageare unchanged, so existing v3.1 gRPC clients/servers remain wire compatible.utils/grcputils.gogained the corresponding JSON↔protobuf conversion functions, following the existingGet*/Set*/Subscribe*/Unsubscribe*pattern.Data-compression cache (wsMgr/udsMgr)
getDcConfignow also recognizes a top-level"data"array (not just the pre-existing"paths"filter variant) as a multi-path response for path/timestamp compression purposes.Test client / fixtures
testClient.godispatches multi-signal get/set to the additive gRPC RPCs anddiscoverto the newDiscoverRequestRPC.testRequests.jsonand the Javascript example commands file (appclient_commands.txt) gained multi-signal and discover examples for ws/mqtt/grpc/uds.README
Documents the new VISSv3.2 features under a new "VISSv3.2 new features" section, and notes that the
v3.2branch now exists alongsidemaster(v3.0/v3.1) andv2.0.Testing
serviceMgrhandler, the new gRPC server methods, and the newgrcputils.goconversion functions.go build ./...andgo vet ./...pass cleanly across the whole repository.go test ./...passes for every package touched by this change, with no new failures. The only failing tests in this environment are pre-existing Windows-only file-permission-mode and timing flakes (TestExportKeyPair_*,wsMgrFTcache tests,TestTouchFile_CreatesWithSafeMode,TestTcpLocalHost) — confirmed identical onmasterbefore this change.Not addressed in this PR (flagged for discussion)
MultiSetRequestMessage(itsValuefield isstring, mirroring the existing single-signalSetRequestMessage), so multi-set struct writes are only available over the JSON-based transports (WS/MQTT/UDS), consistent with how the pre-existingmetadatafilter variant is also unsupported over protobuf.