Add Custom MDC for Afternoon#275
Conversation
…k vehicle JSON from DBCs, drop Vector .mdc binaries Relocate mdc/ and tools/ as sibling can/mdc + can/tools for submodule consumption. Regenerate per-vehicle *.mdc.json from committed DBCs; remove Vector CANdb++ Admin binaries. Co-authored-by: Cursor <cursoragent@cursor.com>
…tron deploy/ Co-authored-by: Cursor <cursoragent@cursor.com>
…rop generated MDC, move lhr-ev1) Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the Modular Description for CAN (MDC) specification, including JSON schemas, validation tools, a DBC importer, and a C header generator. The review feedback identifies several key issues: in dbc2mdc.py, the conversion logic incorrectly discards scaling factors for signals with value tables, and enum attribute default resolution fails when choices are represented as a dictionary. Additionally, the bit-overlap validation in mdc-validate-shared.mjs needs to be updated to consider multiplexer names to prevent false negatives in messages with multiple multiplexers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _conversion(sig: Any) -> dict[str, Any]: | ||
| """Map cantools scale/offset/choices to an MDC conversion object. | ||
|
|
||
| Table conversions intentionally omit scale/offset — only discrete value labels survive. | ||
| """ | ||
| if sig.choices: | ||
| return {"kind": "table"} | ||
| scale = float(sig.scale) | ||
| offset = float(sig.offset) | ||
| if scale == 1.0 and offset == 0.0: | ||
| return {"kind": "identity"} | ||
| return {"kind": "linear", "factor": scale, "offset": offset} |
There was a problem hiding this comment.
In dbc2mdc.py, any signal with a value table (sig.choices) is automatically assigned a "table" conversion kind. However, in the MDC specification, a "table" conversion means the physical value equals the raw value.
If a signal has both a value table (e.g., for specific error states) and a non-trivial scale/offset for its continuous range, forcing "table" conversion completely discards the scale and offset. This results in incorrect physical values being decoded by the engine.
The conversion should only default to "table" if the signal has choices and its scale/offset are identity (1.0 and 0.0). Otherwise, it must use "linear" conversion to preserve the scaling, while still retaining the choices metadata.
| def _conversion(sig: Any) -> dict[str, Any]: | |
| """Map cantools scale/offset/choices to an MDC conversion object. | |
| Table conversions intentionally omit scale/offset — only discrete value labels survive. | |
| """ | |
| if sig.choices: | |
| return {"kind": "table"} | |
| scale = float(sig.scale) | |
| offset = float(sig.offset) | |
| if scale == 1.0 and offset == 0.0: | |
| return {"kind": "identity"} | |
| return {"kind": "linear", "factor": scale, "offset": offset} | |
| def _conversion(sig: Any) -> dict[str, Any]: | |
| """Map cantools scale/offset/choices to an MDC conversion object. | |
| Table conversions intentionally omit scale/offset — only discrete value labels survive. | |
| """ | |
| scale = float(sig.scale) if sig.scale is not None else 1.0 | |
| offset = float(sig.offset) if sig.offset is not None else 0.0 | |
| if sig.choices and scale == 1.0 and offset == 0.0: | |
| return {"kind": "table"} | |
| if scale == 1.0 and offset == 0.0: | |
| return {"kind": "identity"} | |
| return {"kind": "linear", "factor": scale, "offset": offset} |
| def _default_value(defn: Any, raw: Any) -> Any: | ||
| if raw is None or raw == "": | ||
| return None | ||
| if defn.type_name == "ENUM" and isinstance(raw, int) and defn.choices: | ||
| choices = defn.choices | ||
| if isinstance(choices, list) and 0 <= raw < len(choices): | ||
| return choices[raw] |
There was a problem hiding this comment.
In cantools, defn.choices for an ENUM attribute definition can be represented as a dictionary (mapping integer raw values to string labels) or a list of strings.
Currently, _default_value only checks if choices is a list. If it is a dict, the raw integer value is returned unresolved. Since the MDC schema validates enum attributes against a list of string enumValues, returning an unresolved integer will cause schema validation failures.
We should support both dict and list types when resolving the default value.
| def _default_value(defn: Any, raw: Any) -> Any: | |
| if raw is None or raw == "": | |
| return None | |
| if defn.type_name == "ENUM" and isinstance(raw, int) and defn.choices: | |
| choices = defn.choices | |
| if isinstance(choices, list) and 0 <= raw < len(choices): | |
| return choices[raw] | |
| def _default_value(defn: Any, raw: Any) -> Any: | |
| if raw is None or raw == "": | |
| return None | |
| if defn.type_name == "ENUM" and isinstance(raw, int) and defn.choices: | |
| choices = defn.choices | |
| if isinstance(choices, dict): | |
| return choices.get(raw, raw) | |
| elif isinstance(choices, list) and 0 <= raw < len(choices): | |
| return choices[raw] |
| export function simultaneouslyPresent(aIds, bIds) { | ||
| if (!aIds || !bIds) return true; | ||
| return setsIntersect(aIds, bIds); | ||
| } |
There was a problem hiding this comment.
In messages with multiple or nested multiplexers (extended multiplexing), signals can be multiplexed on different multiplexer signals.
Currently, simultaneouslyPresent only checks if the multiplexer ID sets intersect, without verifying if they belong to the same multiplexer signal. If two signals are gated by different multiplexers with disjoint IDs (e.g., Mux1 == 0 and Mux2 == 1), they can actually be active simultaneously. However, the current implementation will falsely assume they cannot be active at the same time, leading to false negatives (undetected bit overlaps).
We should update simultaneouslyPresent and validateBitOverlaps to take the multiplexer name into account.
| export function simultaneouslyPresent(aIds, bIds) { | |
| if (!aIds || !bIds) return true; | |
| return setsIntersect(aIds, bIds); | |
| } | |
| export function simultaneouslyPresent(aMux, aIds, bMux, bIds) { | |
| if (!aIds || !bIds) return true; | |
| if (aMux !== bMux) return true; | |
| return setsIntersect(aIds, bIds); | |
| } |
| const muxIds = | ||
| sig.multiplexer?.role === "multiplexed" ? new Set(sig.multiplexer.ids ?? []) : null; | ||
| const bitSet = new Set(bits); | ||
| for (const prior of placed) { | ||
| if (!setsIntersect(prior.bits, bitSet)) continue; | ||
| if (!simultaneouslyPresent(prior.muxIds, muxIds)) continue; | ||
| const shared = [...bitSet].find((b) => prior.bits.has(b)); | ||
| report("warning", `bit overlap: ${prior.name} and ${sig.name} share bit ${shared}`); | ||
| break; | ||
| } | ||
| placed.push({ name: sig.name, bits: bitSet, muxIds }); |
There was a problem hiding this comment.
Update the bit overlap check to extract the multiplexer name (multiplexorName) and pass it to the updated simultaneouslyPresent function. This ensures that signals gated by different multiplexers are correctly identified as potentially active at the same time.
const muxName = sig.multiplexer?.role === "multiplexed" ? (sig.multiplexer.multiplexorName ?? "") : null;
const muxIds =
sig.multiplexer?.role === "multiplexed" ? new Set(sig.multiplexer.ids ?? []) : null;
const bitSet = new Set(bits);
for (const prior of placed) {
if (!setsIntersect(prior.bits, bitSet)) continue;
if (!simultaneouslyPresent(prior.muxName, prior.muxIds, muxName, muxIds)) continue;
const shared = [...bitSet].find((b) => prior.bits.has(b));
report("warning", `bit overlap: ${prior.name} and ${sig.name} share bit ${shared}`);
break;
}
placed.push({ name: sig.name, bits: bitSet, muxName, muxIds });
No description provided.