feat(sbmd): replace v3 YAML runtime with v4 JavaScript-native architecture by cleithner-comcast · Pull Request #236 · rdkcentral/BartonCore

cleithner-comcast · 2026-06-25T16:46:29Z

Replace the v3 YAML-based SBMD runtime (SbmdParser, SbmdSpec, SbmdScript, ScriptResult) with a v4 JavaScript-native architecture:

SbmdDriver: manages per-driver QuickJS context lifecycle and GC roots
SbmdDispatch: builds and queries handler dispatch tables from driver registrations
SbmdLoader: loads and evaluates .sbmd.js spec files in QuickJS
SbmdHandlerInvoker: marshals C++ context into JS args, invokes handlers, and unmarshals results
SbmdResultExecutor: interprets result chain arrays into C++ side effects
SbmdBundleLoader: assembles and loads the runtime JS bundle (renamed from SbmdUtilsLoader)
SbmdRegistration: header-only types for driver self-registration

Remove the v3 YAML parser, v2/v3 JSON schemas, and dual-engine SbmdScriptImpl. Add v4 JSON schema for spec validation.

Includes comprehensive unit tests for all new components and updated MatterDevice to use the v4 dispatch-based driver model.

Also (unrelated to SBMD v4):

Expose observability metrics (feat(observability): add metrics abstraction with in-mem and noop backends #235) via new b_core_client_get_telemetry public API and getTelemetry command in the reference app.

…cture Replace the v3 YAML-based SBMD runtime (SbmdParser, SbmdSpec, SbmdScript, ScriptResult) with a v4 JavaScript-native architecture: - SbmdDriver: manages per-driver QuickJS context lifecycle and GC roots - SbmdDispatch: builds and queries handler dispatch tables from driver registrations - SbmdLoader: loads and evaluates .sbmd.js spec files in QuickJS - SbmdHandlerInvoker: marshals C++ context into JS args, invokes handlers, and unmarshals results - SbmdResultExecutor: interprets result chain arrays into C++ side effects - SbmdBundleLoader: assembles and loads the runtime JS bundle (renamed from SbmdUtilsLoader) - SbmdRegistration: header-only types for driver self-registration Remove the v3 YAML parser, v2/v3 JSON schemas, and dual-engine SbmdScriptImpl. Add v4 JSON schema for spec validation. Includes comprehensive unit tests for all new components and updated MatterDevice to use the v4 dispatch-based driver model.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

tleacmcsa

OMG...

…spec validation when no specs Replace the raw const SessionHandle* in PendingOperation with a chip::SessionHolder. The deferred result chain re-sends commands and writes on later event-loop turns, after the originating connection callback (which supplied the SessionHandle reference) has returned, so the previous pointer dangled. The holder keeps a lifetime-tracked reference and each deferred send now bails cleanly if the session was released. Also guard the validate_sbmd_specs CMake target so the build does not fail when no .sbmd.js spec files are present (they arrive in a later commit). The validator requires at least one spec argument.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

The comment above CompletePendingOperation in the sendCommand deferred-chain branch was a stream-of-consciousness artifact. Replace it with a concise description of what the code actually does.

ExtractConstants evaluates the constants block as JavaScript, so a string like "hello\nworld" becomes a string containing a real newline byte. GenerateConstantsPreamble emits those values verbatim into var declarations; a bare newline inside a string literal is a JavaScript syntax error that causes the entire driver to fail to load. Re-escape \n, \r, and \t back to their two-character sequences when building the quoted string representation. Add ExtractConstantsControlCharacters unit test to cover this.

The runtime automatically sets RESOURCE_MODE_EXECUTABLE when a resource declares an execute handler. Previously: - docs/SBMD.md omitted "execute" from the modes table entirely, then contradicted itself with a note that function-type resources are "always execute-only" without explaining the mechanism. - openspec/specs/sbmd-system/spec.md had no scenario covering execute mode registration. Add "execute" to the modes table with a note that it is auto-set from handler presence (making it redundant-but-valid to include). Update the function-type resource note to name the auto-set mechanism. Add a spec scenario capturing the auto-set guarantee.

PendingOperation stored a raw MatterDevice pointer set when the deferred operation was parked. If the device was removed before the Matter event-loop response callback fired, ContinueDeferredChain would dereference a dangling pointer. Change PendingOperation::device from MatterDevice ptr to std::weak_ptr<MatterDevice>. ContinueDeferredChain locks the weak_ptr before any terminal that needs the device; if the device has been removed, the operation is failed cleanly.

Address review feedback on the dispatch table: - Rename HandlerPriority::Specific to Single so it no longer collides with specificTable. The value only means "handler declares exactly one alias," which is unrelated to the exact-element keying that specificTable implies. - Rewrite the Build() doc to describe what it actually does: clears the tables, resolves each handler's aliases to (clusterId, elementId) keys, files them into the specific or wildcard table, and skips unknown aliases. - Replace the guard-continue in the alias loop with an if/else two-way split. - Drop the redundant wildcard sort loop; every wildcard entry carries the same priority, so it can never reorder anything. - Trim the sort comment to "(Single < Multi)" (Wildcard never lands in specificTable) and drop the "(for diagnostics)" qualifier from the entry-count accessors.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Follow-up to the HandlerPriority::Specific -> Single rename: update the leftover "specific" test names, handler-name strings, and comments in SbmdDispatchTest to match. The specificTable / GetSpecificEntryCount names are left untouched since they refer to the table, not the priority.

- Add SafeJSValue: an engine-neutral, move-only RAII wrapper that keeps a JS function/value alive for its lifetime and hides the mquickjs moving-GC rooting behind its interface. It is the single abstraction boundary for JS value lifetime; nothing outside the mquickjs backend deals with rooting directly. - Route all SbmdHandlerInvoker arg builders through SafeJSValue (return/accept SafeJSValue instead of raw JSValue). - Store deferred callbacks (ResultTerminal RequestCommand/ReadAttribute and PendingOperation) as SafeJSValue, removing DeferredHandlerRoots and the manual JSGCRef/bool rooted bookkeeping; move-assignment transfers the held reference. - Preserve the deferred handler's opaque context across a re-armed deferred chain: it is established once at the initial trigger and no longer overwritten on each re-arm, so every hop sees the same context until the chain terminates. - Add the ScopedResultRelease RAII guard so leftover parsed callbacks are always released under the runtime mutex on every exit path. - Store each SbmdHandler's held function reference as a move-only SafeJSValue; keep the raw handler JSValue as an unheld load-time staging slot so a not-yet-activated registration owns no held references. - Collapse HandleAttributeReport/HandleEvent/HandleCommand into a shared DispatchToHandlers loop; extract DecodeBase64Tlv/EncodeTlvElementToBase64 and ResolveEndpoint helpers used by ExecuteTerminal and ContinueDeferredChain. - Split supplement handling into PrefetchSupplements (runs the fetchers, no JS context or runtime mutex) and AddSupplements (JS-only, under the mutex), and hoist supplement fetching out of the mutex at the seed, resource, and device-initiated dispatch sites so device-service I/O no longer runs while the runtime mutex is held -- only JS-context access is serialized. - Warn-only handling of outbound terminals on device-initiated reports. - Validate constants keys are JS identifiers in SbmdLoader; add loader tests. - Remove dead PendingOperation fields and no-op wildcard sort; document thread confinement and the held-reference cleanup contract.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

ExtractRegistration reset __sbmd_registration only on the success path. Any early-return after ExtractMetadata, ExtractAliases, ExtractEndpoints, or ExtractDeviceHandlers left the global set, causing the next LoadDriver call to throw "SbmdDriver() called more than once" when it tried to evaluate the following driver. Replace the per-exit-path pattern with a single RAII scope guard that resets the global on every exit -- success or failure. The guard is armed after the initial null check, so it only fires when SbmdDriver() has actually set the global. Add LoadDriverAfterExtractionFailureSucceeds to pin the behaviour.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

tleacmcsa · 2026-07-06T17:34:25Z

-        JSValue args = JS_NewObject(ctx);
+        // Root args for the whole build: subsequent allocations (JS_NewString, JS_NewObject)
+        // may trigger the mquickjs moving GC, which would otherwise sweep the unrooted args.
+        SafeJSValue args {ctx, JS_NewObject(ctx)};


It seems like construction could be a lot nicer for callers: SafeJSValue args(ctx), right?

I'm on the fence. Yes it would be more convenient but it obfuscates allocation. SafeJSValue args {ctx}; doesn't indicate an allocation has occurred and the only place the wrapper does allocate is self-documenting via the AddObject call. A reader could think SafeJSValue args {ctx} is an empty wrapper with a context but it would have a legitimately rooted jsvalue.

I'm not opposed to it I just kinda like the explicit client opt-in to allocation. Will change if you feel strongly though.

cleithner-comcast requested a review from tleacmcsa as a code owner June 25, 2026 16:46

Copilot AI review requested due to automatic review settings June 25, 2026 16:46

cleithner-comcast requested review from kfundecmcsa, mkkoch and rchowdcmcsa as code owners June 25, 2026 16:46

Copilot started reviewing on behalf of cleithner-comcast June 25, 2026 16:46 View session

Copilot AI reviewed Jun 25, 2026

rchowdcmcsa changed the title ~~feat(sbmd): replace v3 YAML runtime with v4 JavaScript-native archite…~~ feat(sbmd): replace v3 YAML runtime with v4 JavaScript-native architecture Jun 25, 2026

tleacmcsa reviewed Jun 25, 2026

View reviewed changes

Comment thread api/c/src/barton-core-client.c

Comment thread core/deviceDrivers/matter/sbmd/mquickjs/SbmdBundleLoader.h Outdated

cleaned up some comments

74248d9