feat: Implement Register/Channel muxing parsing in JMAP

[phako]

No description provided.

[mhier]

I feel the tests are not yet well organised. The testJsonMapFileParser test cases should ideally use the MapFileParser in isolation, without a Device or backend.

Maybe the tests which require a broken jmap file to check for an exception should not bring their own file but instead use an existing file and inject the error into that file (parse the file with nlohman::json, change to make it broken and write to some file with a unique name in the build directory)

Also some test cases seem a bit redundant, but maybe I am just confused by the many possibilities.

[mhier]

I am wondering if the resulting document structure is good. Isn't this a paragraph below the subsection "Muxed channels (selectedBy)"? That points to "channels", so scalar/1D registers should not be explained below that. Maybe simply changing the title of the subsection is enough, although I feel that the per-channel selector is logically more a special case of the register selector?

[mhier]

That ("In a well-formed muxed register the alternatives must resolve to the same address") sounds like a very general restriction. Is this really the case, or is it just pointing out that in this test case with the jmap file at hand this should be the case? We should not restrict the channel layouts between the alternatives to be identical...

[mhier]

Isn't that covered already by the pre-existing named channel slice tests?

Btw: This test case and all test cases which actually obtain accessors should not be in "testJsonMapFileParser.cpp" where we merely want to check the map file parser isolated from the actual backend...

[phako]

Yeah for some reason those tests crept in the wrong file and I did not pay proper attention

[mhier]

Does the channel have a bit field child? Or is the selectedBy target a bit field?

(Channels with bit field children do not work yet, I am working on it right now)

[mhier]

Isn't the linter complaining about the empty {}? It's redundant.

[Copilot]

🟡 Changes recommended

Critical compatibility and fixture issues, along with unresolved selector and validation findings, block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR implements JMAP register/channel muxing with flat channel layouts and selectedBy metadata.

Changes:

• Replaces channelTabs parsing with flat channels layouts.
• Adds selector metadata to registers and channels.
• Updates fixtures, parser/accessor tests, and documentation.

File summaries

File	Reviewed changes and findings
tests/simpleJsonFile.jmap	Updated production-style fixture for the new schema.
tests/selectedByOnlyValue.jmap	Tests a selector with only a value.
tests/selectedByOnlyRegister.jmap	Tests a selector with only a register.
tests/selectedByCases.jmap	Adds selector edge cases. Moderate finding (2 votes): nested channel children are unsupported and silently ignored.
tests/selectedByBadValue.jmap	Tests invalid selector values.
tests/muxedPolled.jmap	Adds a polled mux fixture.
tests/muxedIntegration.jmap	Adds an interrupt mux fixture.
tests/muxedDataAccessor.jmap	Updates the 2D accessor fixture. Critical finding (1 vote): misplaced braces discard DBL and DOUBLE_BUF, and remaining channelTabs data must be migrated.
tests/executables_src/testJsonMapFileParser.cpp	Adds parser and accessor tests. Three moderate findings (1 vote each) identify incorrect channel guards; three nit findings (1 vote each) identify stale exception-contract comments.
src/NumericAddressedRegisterCatalogue.cc	Includes selector metadata in equality handling.
src/JsonMapFileParser.cc	Parses flat channels and selectors. Critical finding (2 votes): removing channelTabs breaks the existing converter. Moderate findings: register selectors are not propagated to bit-field children (2 votes), and explicitly empty channels maps are accepted (1 vote).
include/NumericAddressedRegisterCatalogue.h	Exposes selector metadata structures.
doc/jmapFormat.dox	Documents the revised JMAP format.

Review details

Suppressed comments (9)

src/JsonMapFileParser.cc:289

• Checking only channels.empty() loses the distinction between an absent channels member (scalar/1D) and an explicitly empty 2D definition. The previous parser rejected an empty channel definition, but { "channels": {} } now silently takes the scalar path and ignores its 2D fields such as pitch; preserve presence information or reject an explicitly empty channels map.

          if(channels.empty()) {
            auto bPerElem = (bytesPerElement != 0 ? bytesPerElement : 4); // create default if not set

tests/executables_src/testJsonMapFileParser.cpp:497

• This guard checks channel 0, but the next two assertions dereference reg.channels[1].selectedBy. If channel 1 is missing its selector, the test can dereference an empty optional instead of reporting the intended failure; guard the same channel that is about to be used.

    BOOST_REQUIRE(reg.channels[0].selectedBy);

tests/executables_src/testJsonMapFileParser.cpp:515

• This guard checks channel 0, but the following assertions dereference reg.channels[3].selectedBy. If channel 3 is missing its selector, the test can dereference an empty optional instead of reporting the intended failure; guard the same channel that is about to be used.

    BOOST_REQUIRE(reg.channels[0].selectedBy);

tests/executables_src/testJsonMapFileParser.cpp:506

• This guard checks channel 0, but the following assertions dereference reg.channels[2].selectedBy. If channel 2 is missing its selector, the test can dereference an empty optional instead of reporting the intended failure; guard the same channel that is about to be used.

    BOOST_REQUIRE(reg.channels[0].selectedBy);

tests/executables_src/testJsonMapFileParser.cpp:727

• This note is now stale: SelectedBy::from_json explicitly rejects a missing value, and MapFileParser::parse wraps that as ChimeraTK::logic_error. Please make the comment describe the implemented contract instead of calling the deserializer lax.

// selectedBy with only 'register', no 'value'. Desired semantics (documented): both fields are required, so the
// parser must reject the map with std::logic_error (a missing 'value' must not silently default). NOTE: the current
// lax deserializer does not yet enforce this; this test documents the desired behaviour.

tests/executables_src/testJsonMapFileParser.cpp:735

• This note is also inaccurate: the custom deserializer now rejects a missing register and the outer parser converts the error to ChimeraTK::logic_error. Update the comment so it does not claim the behavior is only desired or not guaranteed.

// selectedBy with only 'value', no 'register'. Desired semantics (documented): the parser must reject the map
// because both fields are required. NOTE: expected std::logic_error, not guaranteed by the current lax deserializer.

tests/executables_src/testJsonMapFileParser.cpp:758

• The implementation now catches JSON type errors in MapFileParser::parse and rethrows ChimeraTK::logic_error, so this note no longer describes the observed behavior. Replace the claim about a nlohmann::json exception with the actual public exception contract.

// selectedBy with a non-numeric 'value' (e.g. a string) must be rejected with std::logic_error. NOTE: currently a
// nlohmann::json type error surfaces instead; this test documents the desired behaviour.

tests/muxedDataAccessor.jmap:479

• The DBL entry immediately following this migrated block still uses the removed channelTabs schema. Once the extra brace is fixed, the new parser will not populate its 2D channels, so existing tests that open /TEST/DBL.1 and its buffer views will fail. Convert this entry to the flat numberOfElements/pitch/channels form too.

        }

tests/muxedDataAccessor.jmap:480

• This leaves muxedDataAccessor.jmap invalid JSON: line 479 closes NODMAASYNC, and the unchanged }, on line 480 closes the same parent again before DBL. Every existing test that loads this fixture will fail before exercising the accessor. Keep one closing brace and put the comma on it.

        }
        },

• Files reviewed: 13/13 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.