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.

[killercup]

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! 😉

[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 <#33 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE4XYPBPRYX36ML2TRSILTF43SFANCNFSM4Z4FWXKQ> .

[killercup]

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?

[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]

Thanks for the quick update again!

[killercup]

Thanks for the quick update again again!

I saw you fixed some clippy errors, can you also fix the "use of or_insert followed by a function call" ones?

[killercup]

Hmm, looking at this again, do you think we should include the mulitplexor value? Can we always convert it to a u16 or something? Upside: Better debugging. Downside: Larger size of CanError type.

[marcelbuesing]

Yes I think it's safe to say that it should be possible to convert into u16. I would probably even assume you'll never need more than u8. I think it certainly helps to add the value, otherwise it's hard to debug. So I think it's worth the downside. Went with u16 now, but like i said i can not imagine a use case that requires more than u8 and if there is such a use case that would at least to a compile time error with the generated code.

[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!