Multiplexed signal implementation

[marcelbuesing]

This is currently work in progress. Whats basically missing right now, besides a cleanup, is adding the signal fns to the multiplex structs.

[marcelbuesing]

Some drive-by comments

Like the apporoach of constructing enums with a ref to the raw data slice. I would however like to point out that referencing what is at most 8 bytes with a fat pointer is in the best case (on 32bit ARM) as fast as just copying the whole slice! wink

I think for standard can frames I agree it's probably not worth passing refs compared to copying. It looks like (example here shows a message with 33 bytes payload) dbc-files is maybe used with CAN-FD (max 64 bytes) or ISO-TP (max 4095 bytes) though. Copy nevertheless as it's maybe an edge case?

[killercup]

I had assumed that we support just the boring old CAN frames for now, but you raise a good point. If it's not a big deal and will make this more future proof, let's go with using references :)

On Sun, 28 Mar 2021, 15:22 Marcel, @.***> wrote:

Some drive-by comments

Like the apporoach of constructing enums with a ref to the raw data slice. I would however like to point out that referencing what is at most 8 bytes with a fat pointer is in the best case (on 32bit ARM) as fast as just copying the whole slice! wink

I think for standard can frames I agree it's probably not worth passing refs compared to copying. It looks like https://github.com/marcelbuesing/dbcc/blob/7e3e6b7e608563aa52c1ace143c1194d63b0c183/examples/j1939.dbc#L1511 (example here shows a message with 33 bytes payload) dbc-files is maybe used with CAN-FD (max 64 bytes) or ISO-TP (max 4095 bytes) though. Copy nevertheless as it's maybe an edge case?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/technocreatives/dbc-codegen/pull/33#issuecomment-808896524, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAE4XYPBPRYX36ML2TRSILTF43SFANCNFSM4Z4FWXKQ .

[marcelbuesing]

Hey, sorry for dropping this over easter! How far along is this? I've added some small comments here and there, but I also saw you had a TODO comment. Want to get this into a state where we can merge it even if it's not 100% complete?

It's usable now, still needs tests against the cantools implementation for verification. I am not particularly happy with the way setting multiplexed signal variants works. See the all.rs pack_unpack_message_containing_multiplexed_signals for a demo. It'd be great if anyone has a better idea for this. The only alternative I see right now is passing some lambda that modifies the values. E.g. something along:

set_M0(|m0| {
   mo.set_multiplexed_signal_zero_a_raw(1.2)?
      .set_multiplexed_signal_zero_b_raw(2.0))?;
   Ok(())
}

[killercup]

Wow, amazing work!

Fist off: Why did you change all setters to _raw? IIRC the raw getter is only useful when you want to raw bits from an enum representation. I'd argue that the default should always be the non-raw version.

Thinking about setters, I'm again conflicted about the borrowing of the data… I think the ideal API is msg.set_M0(MultiplexTestMultiplexorM0::new(1.0. 1.5)?). This does not require creating a signal with a default value and changing it in a closure as it is correct by construction. Do you agree?

We can achieve this by either making the enum own its bytes, or by making it a wrapper around Cow. I'd go for making everything more simple and just use the owned data until we run into issues :)

[marcelbuesing]

Wow, amazing work!

Fist off: Why did you change all setters to _raw? IIRC the raw getter is only useful when you want to raw bits from an enum representation. I'd argue that the default should always be the non-raw version.

Thinking about setters, I'm again conflicted about the borrowing of the data… I think the ideal API is msg.set_M0(MultiplexTestMultiplexorM0::new(1.0. 1.5)?). This does not require creating a signal with a default value and changing it in a closure as it is correct by construction. Do you agree?

We can achieve this by either making the enum own its bytes, or by making it a wrapper around Cow. I'd go for making everything more simple and just use the owned data until we run into issues :)

I reverted the _raw suffix changes. I think it would be great in the future to have a more convenient, safer new fn that takes e.g. enums as parameters where available and the same for setters. Anyway this should not be part of this PR see #39 .

Regardings the setters, I will try it with owned data and see how it turns out, I agree for now it probably should not be an issue and it's better to change it later when the need comes up.

[marcelbuesing]

Ok I changed it, I think it looks better now. Any ideas how to do the whole thing without bitvec::BitArray for bitwise or ?

[killercup]

I think this is good to merge besides the small comments above. What do you think?

[marcelbuesing]

I think this is good to merge besides the small comments above. What do you think?

Yes I think it's good now, besides the error type u8/u16 question it should be fine now. I fixed the clippy warnings. Besides that I removed the associated switch index constant as now one can not set the raw value of multiplexor anyway, I don't think it adds value otherwise.

[killercup]

Alright, let's merge this then!

Thanks again for working on this for weeks -- super impressive!