Repository navigation
Fix for Issue #205 (Overlapping Signals / Multiplexors) - #825
friessssss wants to merge 5 commits into
Conversation
andlaus
left a comment
There was a problem hiding this comment.
I did not go into this too deeply, but overall it looks good. I'm wondering if this works with multiplexers, though...
| | int.from_bytes(formats.little_endian.pack(raw_signal_values), "little") | ||
| ) | ||
|
|
||
| for big_endian, little_endian in formats.extra_layers: |
There was a problem hiding this comment.
please add a comment about what extra layers are
| try: | ||
| return bitstruct.c.compile(fmt, names) | ||
| except Exception: | ||
| return bitstruct.compile(fmt, names) |
There was a problem hiding this comment.
without importing bitstruct, this will crash as well. please use the previous approach and simply call bitstruct.compile()
There was a problem hiding this comment.
I moved this to a function for ease of re-use. Returned the initial import to the original approach, so this should be much more valid now.
There was a problem hiding this comment.
since bitstruct.c is imported as bitstruct (if available), this function is redundant: simply call bitstruct.compile().
There was a problem hiding this comment.
I don’t think calling bitstruct.compile() directly works here. import bitstruct.c doesn’t rebind bitstruct to the C module. Like any import a.b, it binds the name bitstruct to the top-level package, which is the pure-Python one. I checked: after import bitstruct.c, bitstruct.compile is a Python function, while bitstruct.c.compile is the built-in. So bitstruct.compile() would quietly drop the C speedup for every message.
The fallback is also still needed. bitstruct.c may not be installed, and when it is, it doesn’t support every format. For example, bitstruct.c.compile('u65', ...) raises NotImplementedError, which can happen with long diagnostic DIDs.
This is the same try/fallback master already had (once for big-endian, once for little-endian). I moved it into a helper because it’s now called once per layer. I added a docstring explaining this.
There was a problem hiding this comment.
what I meant was to import bitstruct like this:
try:
import bitstruct.c as bitstruct
except ImportError:
import bitstructif done like this, bitstruct.compile() should automatically select the correct module. (assuming that no features that are unsupported by bitstruct.c are used, I think this is the case in cantools.)
There was a problem hiding this comment.
I tried import bitstruct.c as bitstruct, but cantools does compile formats that bitstruct.c doesn’t support. With that import, 38 tests fail:
- Signals wider than 64 bits (e.g. tests/files/kcd/message_layout.kcd has a 128-bit signal) raise
NotImplementedError. Whether the C version works depends on the format string, so the fallback has to happen when each format is compiled, not at import. - The little-endian padding mask uses
bitstruct.pack(f'u{length}', ...)over the whole message, so anything longer than 8 bytes fails to load. bitstruct.chas noError, so theexcept bitstruct.Errorindecode_datawould raiseAttributeError.
I’ve kept the per-format try/fallback from master, let me know if that’s okay with you ! I'm not totally against the switch but it might need some more ironing out.
|
@andlaus I added an explicit multiplex test to the test suite to make sure that this works as expected. Also the |
|
We would like to see this merged. Our DBCs describe status bitfields twice: once as the whole value and once as single-bit signals, so that tools like CANoe show both the raw word and the named flags. CANoe handles this fine, but cantools cannot decode these messages today, so we cannot use the same DBCs in our Python tooling. I tested this branch, merged onto current master, with two of our DBCs (68 and 6 overlapping pairs, all inside a multiplexer branch). On master both fail to decode with Two points:
|
|
Following up on encoding: I put a proposal in a draft PR in my fork, on top of this branch: ckagerer#4. Its diff shows only the change on top of this PR. The rule: overlapping signals within the root or one multiplexer branch must have the same raw bits where they overlap, else One cost: decode, change one view, and encode again raises until the other views are changed or removed. With the current OR, clearing a bit that way gives a wrong frame without an error. If this direction is fine, I can open it as a follow-up PR after this one is merged. |
| little_fmt, little_padding_mask, little_names = create_little() | ||
| def compile_layer( | ||
| be_signals: Sequence[Union["Data", "Signal"]], | ||
| le_signals: Sequence[Union["Data", "Signal"]], |
There was a problem hiding this comment.
this should be two functions: compile_be_layer(), and compile_le_layer(). This would avoid the shenanigans below.
There was a problem hiding this comment.
Done. It is now compile_be_layer() and compile_le_layer(), which return (codec, padding_mask).
| for index in range(layer_count) | ||
| ] | ||
|
|
||
| padding = compiled_layers[0].padding_mask |
There was a problem hiding this comment.
a comment what is done here and why would be good.
There was a problem hiding this comment.
The padding mask marks bits that no signal uses. Message.encode(padding=True) fills exactly those bits with unused_bit_pattern, so the mask must not include any bit that belongs to a signal.
With overlapping signals, each layer is compiled separately and only knows about its own signals. Its mask therefore treats every other layer’s bits as padding. A bit is really unused only if it is padding in all layers, so the masks are combined with AND.
Here’s what goes wrong with only the first layer’s mask (the old primary-codec behaviour). Take a 1-byte message with Low on bits 0–3 and Wide on bits 2–5, with unused_bit_pattern=0xff. They overlap, so they land in different layers:
Layer 1 (Low) has mask 0b11110000. Bits 4–5 look like padding, but Wide uses them.
Layer 2 (Wide) has mask 0b11000011.
ANDed, the mask is 0b11000000. Only bits 6–7 are really unused.
Encoding {'Low': 0, 'Wide': 0} with padding=True gives 0xc0 with the combined mask. With layer 1’s mask alone it gives 0xf0, which decodes as Wide == 12. The value is silently corrupted.
I expanded the code comment to explain this and added test_overlapping_signals_padding for this case.
| padding_mask: int | ||
| # Additional (big, little) codecs for overlapping signals. Each | ||
| # layer is internally non-overlapping so bitstruct can unpack it. | ||
| extra_layers: tuple[tuple[CompiledFormatDict, CompiledFormatDict], ...] = () |
There was a problem hiding this comment.
why not simply use lists for everything? i.e., something like
big_endian_layers: list[CompiledFormatDict]
little_endian_layers: list[CompiledFormatDict]
if there are no overlapping signals, these lists exhibit length 1.
There was a problem hiding this comment.
Formats now has big_endian_layers and little_endian_layers lists, each with one entry when nothing overlaps
|
@friessssss: are you gonna push this or do you prefer if I take it over? This is a long-standing major missing feature which I'd like to get merged in the not too distant future... |
|
@andlaus I will knock out these changes today and test them out ! I'd love to get this in ASAP ! |
- Formats now holds big_endian_layers / little_endian_layers lists instead of a primary codec pair plus extra_layers - split compile_layer() into compile_be_layer() / compile_le_layer() - document _compile_bitstruct() and the padding mask computation - add a dump/load round-trip test for an overlapping DBC
|
almost there. I think that we should consider adding support for negative padding in bitstruct/bitstruct.c as it would make implementing overlapping signals in cantools significantly simpler and probably more performant. (I have a patch for that, but I did not yet find the time to submit it.) |
|
ok, negative padding is not going to fly with bitstruct, so I think we should go the route which you proposed. IMO the PR is ready, once the |
|
@friessssss: are you going to try the import-level |
|
@andlaus I left you a quick comment back on that ! Let me know which direction you want to go on this, happy to make the change wit we decide thats the best way to go ! |
|
you mean this one?
The unavailability of |
Summary
Fixes #205.
cantools still treated overlapping signals as invalid at decode/encode time, even when a database was loaded with
strict=False. That shows up asValueError: Short data(or aDecodeErrorwrapping it).This is a real pattern in automotive DBs: bit flags plus a wider status/fault field covering the same bits. The original report used eight 1-bit flags overlapping an 8-bit
Statuson a 1-byte Intel message. The same failure also happened for Motorola signals and for multiplexed SYM frames where a status word shares bits with a flag.strict=Trueis unchanged: overlapping signals still raise at load time. Withstrict=False, decode and encode now return consistent values for both the wide field and the overlapping bits.Approach
bitstructcannot represent overlapping fields in a single format string. Overlapping signals are now split into internally non-overlapping layers, each compiled as its own codec. Decode unpacks every layer and merges the results. Encode packs each layer and ORs them together.Non-overlapping messages still use a single layer, so the common path is unchanged.
Tests
tests/files/sym/issue_205.sym)Notes
Load overlapping databases with
strict=False, same as before. Encoding also needsstrict=Falseso the overlap check inMessage.encode()does not reject the payload.