Basic parsing for extended multiplexing

[Uight]

Solves #35 in some part. Its the most basic parsing possible. i let the code to parse the ranges in there but until the interface for this is defined i wont parse it further or do any kind of calculating usefull properties for the extended multiplexing.
As it is now user would need to further parse the info.

[Adhara3]

I haven't merged this one yet because I want to understand a couple of things first.

[Uight]

@Adhara3 based on this parsing? or general Api related?

[Adhara3]

@Adhara3 based on this parsing? or general Api related?

Multiplexed related. The parsing is fine but it hurts me not to expand ranges into proper classes, i.e. I would like not to keep the full string.
Moreover, I would like to understand the relationship between m2M kind of multiplexing identifier to see if we can embed this advanced info into the MultiplexingInfo.
Do you need a release to use it as it is for your company? If so let's release as it is

A

[Uight]

In my original implementation the ranges where in a class and i also used the same class for simple multiplexing where the lower and upper range is always the same.
However just having the ranges is still not a good information. As i stated in the API discussion what you need is:

Message knows if it contains multiplexed signals or not;
Signals knows if it is multiplexed or not via a enum (None, Multiplexor, Multiplexed, MultiplexedMultiplexor)
Every multiplexor(also MultiplexedMultiplexor) should know which signals are multiplexed by it and with what value or range(s).
Every mutliplexed signal should know its own Multiplexor and the value or range(s) at which it is expected.

This can only be calculated when finalizign the dbc so i think just saving the string is fine for now at least.

As for the release i would like to have the byte packing stuff in a release that would be enough for now. This extended multiplexing stuff is more or less only a prep for message packing/unpacking if the message contains multiplexors.

as for a m2M this can only happen when a message/signal has extended multiplexing. The 2 is mostly useless in there as i think all editors just write the lower number or the first range in the extendedn multiplexing in there. the "m" marks that its a multiplexed signal just as in the simple multiplexing. However in extended multipleing there can be multiple first level multiplexors (marked with just "M") To see which on the signal actually uses the SG_MUL_MUL definitions have to be parsed.
The "M" at the end just marks that the signal itself is a multiplexor that can be referenced by any other multiplexed signal in the message through another SG_MUL_MUL definition.

I think the whole Multiplexing info should be updated to something like this (pseudo):

public class MultiplexingInfo(whatever is needed to calc it)
{
MultiplexingRole (None, Unknown(for parsing fails), Multiplexor, Multiplexed, MultiplexedMultiplexor)

MultiplexedSignals (List type for all signals multiplexed by a multiplexor) Empty or null if not a multiplexor or MultiplexedMultiplexor (could also contain the relevant ranges for each signal too)

Multiplexingranges (List type containing a List of ranges for the multiplexor value when this value is used) empty or null if not multiplexed.

Multiplexor (either the name or the whole signal (ref)) null or string.empty if not a multiplexed signal.
}

public class Multiplexingrange()
{
bool IsRange (indicating wether both values are the same or not)
uint lower
uint upper
}

Overall generating this requires a double pass. First calc fill the role an dthe multiplexing role and the multiplexor and in a second pass all signals can register to a multiplexor to fill the (MultiplexedSignals property up). Could be done in one pass by allowing a signal to append itseld to MultiplexedSignals even if the signal it appends to is not yet defined a multiplexor.
(Can also be done by parsing in the correct order: First all that are only "M" then all "mxM" and then all "mx"

[Uight]

Should now line up a with the direction the immutable stuff i going in;

However im still unsure how the Multiplexing Info class should look. i can agree on not putting all infos (like ref from multiplexor to multiplexed signals in) in the object.

Currently this would mean changing to
public class MultiplexingInfo
{
public MultiplexingRole Role { get; } //always set
public IReadOnlyCollection ranges { get; } //set for all Multiplexed values
(public string Multiplexor { get; } //Set for all multiplexed values (only possible for extended as is)
}

for the simple multiplexing supported now this would just have a range with the value in both lower and upper.
However for extended Multiplexing theres one important thing missing here which is the Multiplexor signal. As thats a string so nullable that would be fine but what i dont like is that i can only set it for extended Multiplexing. For normal multiplexing i can not get the multiplexor signal from just the signal definition. Youd need the Message for that.
i dont like that it would be null in one case and filled in the other. I think extended and normal multiplexing should behave the same.

[Adhara3]

What about expanding the valid values?
So that 1-1, 5-7 is exploded into [1, 5, 6, 7].
I was wondering if that would simplify, after all range or not, from a decoding point of view, we only need to read a Muxer value and pick the right layout based on that value.

Just reasoning

[Adhara3]

i dont like that it would be null in one case and filled in the other. I think extended and normal multiplexing should behave the same.

yeah, I agree they should be equivalent after parsing is over or, looking for a different perspective, the demuxing code should be unaware if coming from normal or extended

For normal multiplexing i can not get the multiplexor signal from just the signal definition. Youd need the Message for that.

I see your point here but it may be related to current Packer implementation having a Signal in the signature? If yes, then I may argue that this signature should be used only for fixed packet layout, which is not the case for muxed ones. Moreover, when you read you do have the full message.

Bottom line, concerns are clear, we'll find a way

A

[Adhara3]

Again, just wondering

var triggerValues = new List<uint>();

foreach (var range in rangesArray)
{
    var rangeClean = range.Trim();
    var numbers = rangeClean.Split('-');

    var lower = uint.Parse(numbers[0]);
    var upper = uint.Parse(numbers[1]);
    
    // Maybe check if upper >= lower?
    rangesParsed.AddRange(Enumerable.Range(lower, upper - lower + 1));
}

[Uight]

theres so much that could be checked here:

1. Upper is higher or than lower
2. the mux value in the signal definition (m2M) is in the ranges?
3. Is the every mux value possible from the muxor?
4. is the muxor a integer? (or bool which is also an integer in dbc scope)
5. duplicates?

I would go with this:

1. check if upper is higher then lower or else parsing error?
2. use something like a try add to ignore duplicates (not sure atm but ISet is a hashset ? should have a try add just like dictionary)
3. add the value from the signal (m15M) <= 15 in this case too.

for the final message parsing having ranges sucks anyway it way slower than checking i hashset on contains.

[Adhara3]

Or should mark MultiplexingRole as [Flags] and have a multiplexed multiplexor be a Multiplexed | Multiplexor?
I have mixed feelings: one one hand it makes easier to spot how is muxed and who is muxer, on the other hand we should remove the Unknown value because it can't be mixed.

[Adhara3]

Let's say we have instead:

internal class ParsingGeneralMultiplexing
{
  public MultiplexingRole Role{ get; set; }

  /* 
       For muxers the below is always null
       For standard mux info, the following is null at the end of the parsing but can be filled in the finish up
       phase by picking the only muxer expected in the message (if more than one, parsing error). Moreover for standard 
       the multiplexer value is filled with a single element, what is called group in the multiplexing info
       For advanced mux multiplexor signal is filled and multiplexer values are replaced (SG_ parsed values should be 
       overwritten)
  */
  public string MultiplexorSignal { get; set;}
  public ISet<uint> MultiplexerValue { get; } 

  // May be handy for checks in the wrap up phase. Should not be reported in the immutable public object
  public bool Extended {get; set;}
}

So, simple one

 BO_ 2348875519 SimpleMX_Message: 5 Vector__XXX
   SG_ S7 : 36|4@1+ (1,0) [0|0] "" Vector__XXX
   SG_ S5 m4: 4|24@1- (1,0) [0|0] "" Vector__XXX
   SG_ S4 m3: 4|24@1- (1,0) [0|0] "" Vector__XXX
   SG_ S3 m2: 20|16@1- (1,0) [0|0] "" Vector__XXX
   SG_ S2 m2: 4|16@1- (1,0) [0|0] "" Vector__XXX
   SG_ S6 m1: 2|32@1- (1,0) [0|0] "" Vector__XXX
   SG_ S0_m M : 0|2@1+ (1,0) [0|0] "" Vector__XXX

Phase 1: SG parsing

S7 will have role None, S0_m role Multiplexer, the other role Multiplexed and multiplexer value 1 | 2 | 3 | 4.

Wrap up

In the FinishUp or WrapUp phase, S0_m is forced into MultiplexerSignal for all Multiplexed for that message since only one M is present

The extended

BO_ 2348875518 ExtMX_Message: 5 Vector__XXX
  SG_ S7 : 36|4@1+ (1,0) [0|0] "" Vector__XXX
  SG_ S5 m6 : 4|24@1- (1,0) [0|0] "" Vector__XXX
  SG_ S4 m5 : 4|24@1- (1,0) [0|0] "" Vector__XXX
  SG_ S3 m4 : 20|16@1- (1,0) [0|0] "" Vector__XXX
  SG_ S2 m4 : 4|16@1- (1,0) [0|0] "" Vector__XXX
  SG_ S6 m1 : 2|32@1- (1,0) [0|0] "" Vector__XXX
  SG_ S1_m m0M : 2|2@1+ (1,0) [0|0] "" Vector__XXX
  SG_ S0_m M : 0|2@1+ (1,0) [0|0] "" Vector__XXX

SG_MUL_VAL_ 2348875518 S5 S1_m 6-6;
SG_MUL_VAL_ 2348875518 S4 S1_m 5-5;
SG_MUL_VAL_ 2348875518 S3 S1_m 4-4;
SG_MUL_VAL_ 2348875518 S2 S1_m 4-4;
SG_MUL_VAL_ 2348875518 S6 S0_m 1-1;
SG_MUL_VAL_ 2348875518 S1_m S0_m 0-0, 2-2;

This becomes a 3-phase stuff.

Phase 1: SG parsing

As the standard one, values will contain numbers extracted from standard muxing (i.e. m5, m0M) since at this stage we do not know if SG_MUL_VAL_ will be present.

Phase 2: SG_MUL_VAL_

We set the muxor name and we clear and fill values with ranges exploded.
If needed we can set Extended to true

Wrap up

Names are filled, so no action needed.

Layouts

For unpacking this whole information should be converted into something more handy that I would call layouts and look like what CANdb++ is showing. The idea is that not the user, but the message itself can unpack a byte[] of data autodetencting the layout by reading muxer values and using the right layout to read.
So, using the above example:

Read S0_m value. 
  if 1, then the layout will include S0_m, S7 and S6
  If 0 or 2, the read S1_m
    If 4 layout is S0_m, S7, S1_m, S2 and S3
    If 5 layout is S0_m, S7, S1_m, S4
    If 6 layout is S0_m, S7, S1_m, S5

I like this abstraction because the procedure above can be set up by the lib itself in the wrap up phase so having even more info that what we want to expose in the public API.
Moreover a layout il nothing but a virtual message definition. A multiplexed message can be exploded into several message definitions with no muxed signals in it, the logic part is how to retrieve the correct one given a byte[] but that should not be rocket science, it's just a matter of ordering the muxers and reding values.

These are more or less my ideas, I hope that they are clearer with this example.

Cheers
A

[Uight]

Best way forward for me would be if i wait for the proof of concept for immutability go thorugh as i would have to do pretty much the same stuff as in there to achieve that and rebase this after that.

main resons being:
i would like to also have the extension method for Multiplexing Info internal;
i need the message when building signal multiplexing;
The current code would be easy to merge as its mainly new files anyway

Some remarks on your comment:
"by picking the only muxer expected in the message (if more than one, parsing error)" => not sure how you would do that. You want to have a real exception or expose the parsing observer to the builder functions or create a builder observer? My idea would have been to set everything to unknown if that happens witch unknow marking an error but in addition you could still create an entry in the observer.
"public ISet MultiplexerValue { get; } " you expect this to contain one element in case of normal multiplexing and 2*n elements for n ranges in extended multiplexing?
=> signal with multiplexing "m2M" is parsed then it would conatin "2" and if then a extended mutiplexing is found specififying ranges 2-10, 15-15 you would replace it with "2","10","15","15"?

To assign the single multiplexor to any signal (in simple multiplexing mode) i still need some strange logic i dont like. the message parses signals one by one and each signals builds it multiplexing info. Either pre parse some info that can then be using in calculating the multiplexing info or add the value later

for the packer stuff i agree i would probably reverse the multiplexing tree and always start with the multiplexors and see which signals they apply to.

[Adhara3]

Best way forward for me would be if i wait for the proof of concept for immutability go thorugh as i would have to do pretty much the same stuff as in there to achieve that and rebase this after that.

Yep. If we are fine with the immutability poc I can merge that. That will have some API changes, but if we think it's the first step....

not sure how you would do that. You want to have a real exception or expose the parsing observer to the builder functions or create a builder

Generally speaking, if the dbc is flawed, we should fail parsing. I prefer that that say all good and then decode the wrong stuff. I would not throw, but use the parsing observer

=> signal with multiplexing "m2M" is parsed then it would conatin "2" and if then a extended mutiplexing is found specififying ranges 2-10, 15-15 you would replace it with "2","10","15","15"?

no, would replace with {2, 3, 4, 5, 6, 7, 8, 9, 10, 15}

To assign the single multiplexor to any signal (in simple multiplexing mode) i still need some strange logic i dont like. the message parses signals one by one and each signals builds it multiplexing info. Either pre parse some info that can then be using in calculating the multiplexing info or add the value later

I do not like that logic either but that's a dbc format limitation, it's not indexed. Moreover, in parsing reading the file is most expensive thing, so pre parsing is weird the same way. The disconfort here is minimal and if we keep the pain in the parsing phase then fixing in the wrap up phase we have to safe places for the checks and the public item is fine, which is the most important thing. With the SG_ parsing it's intrinsic that the multiplexer is unknown until the message parsing is over (for simple mux) and until the file is over (for advanced).

Anyway, the way we handle parsing and wrap up can be changed anytime without affecting the user, so it doesn't need to be perfect out of the box. As long as we provide a consistent user API, how we manage our internals can be changed any time so to me it is a bit lower priority to agree on a solution, especially for details.

A

[Adhara3]

I managed to have a working solution using my above suggestions.
In the wrap up phase I am able to build a message decoder for each message. I expose it like this:

public interface IDataHandler
{
    void Handle(uint messageId, byte[] data, Action<Signal, double> signalData);
}

The user that received a CAN message can call this method using the message ID and the byte[] of data. With this implementation, we no longer need the IEnumerable<Message> to become a IReadOnlyDictionary<uint, Message> since the access is now hidden. By the way, behind the scenes I am not accessing a Message but a ISignalProcessor that has ben built in the process and represents the pipeline.

Using the extended multiplexing example above (with a fix)

BO_ 2348875518 ExtMX_Message: 5 Vector__XXX
  SG_ S7 : 36|4@1+ (1,0) [0|0] "" Vector__XXX
  SG_ S5 m6 : 4|24@1- (1,0) [0|0] "" Vector__XXX
  SG_ S4 m5 : 4|24@1- (1,0) [0|0] "" Vector__XXX
  SG_ S3 m4 : 20|16@1- (1,0) [0|0] "" Vector__XXX
  SG_ S2 m4 : 4|16@1- (1,0) [0|0] "" Vector__XXX
  SG_ S6 m1 : 2|32@1- (1,0) [0|0] "" Vector__XXX
  SG_ S1_m m0M : 2|2@1+ (1,0) [0|0] "" Vector__XXX
  SG_ S0_m M : 0|2@1+ (1,0) [0|0] "" Vector__XXX

SG_MUL_VAL_ 2348875518 S5 S1_m 3-3;
SG_MUL_VAL_ 2348875518 S4 S1_m 2-2;
SG_MUL_VAL_ 2348875518 S3 S1_m 1-1;
SG_MUL_VAL_ 2348875518 S2 S1_m 1-1;
SG_MUL_VAL_ 2348875518 S6 S0_m 1-1;
SG_MUL_VAL_ 2348875518 S1_m S0_m 0-0, 2-2;

and feeding with random numbers

var random = new Random();
var data = new byte[64];

for (var i = 0; i < 50; i++)
{
    TestContext.Progress.WriteLine($"\nStarting iteration {i+1}");
    random.NextBytes(data);

    handler.Handle(message.ID, data, (signal, d) =>
    {
        TestContext.Progress.WriteLine($"{signal.Name} --> {d}");
    });
}

this is the outcome:

Starting iteration 11
S7 --> 5
S6 --> 2.8567148955303722E-21
S0_m --> 1

Starting iteration 12
S7 --> 1
S0_m --> 3

Starting iteration 15
S7 --> 7
S4 --> 3365834
S1_m --> 2
S0_m --> 2

Starting iteration 17
S7 --> 9
S5 --> 7217907
S1_m --> 3
S0_m --> 2

Starting iteration 21
S7 --> 8
S3 --> 16769
S2 --> 27314
S1_m --> 1
S0_m --> 2

This first implementation is still using Packer but can be further optimized by also pre build the signal data extractor.
A

[Uight]

The user that received a CAN message can call this method using the message ID and the byte[] of data. With this implementation, we no longer need the IEnumerable<Message> to become a IReadOnlyDictionary<uint, Message> since the access is now hidden.

With the plan to create a seperate packer from the parsed objects in model. you can do whatever you want with the messages in the dbc object.
Maybe im just not gettign where you setup the interface. youd still search the corresponding message from the id somewhere? and for a search a dictionary is the fasted (fastest somewhat reasonable) because its based on a hashtable.

For unpacking this whole information should be converted into something more handy that I would call layouts and look like what CANdb++ is showing. The idea is that not the user, but the message itself can unpack a byte[] of data autodetencting the layout by reading muxer values and using the right layout to read.
So, using the above example:
Read S0_m value.
if 1, then the layout will include S0_m, S7 and S6
If 0 or 2, the read S1_m
If 4 layout is S0_m, S7, S1_m, S2 and S3
If 5 layout is S0_m, S7, S1_m, S4
If 6 layout is S0_m, S7, S1_m, S5

Starting iteration 21
S7 --> 8
S3 --> 16769
S2 --> 27314
S1_m --> 1
S0_m --> 2

From the way the lines are printed i could suspect that the values are double parsed? or do you save them internally? i would expect S7 (no multiplexor) and S0_m to be always the first values as you would need s0_2s values for all others

[Adhara3]

From the way the lines are printed i could suspect that the values are double parsed? or do you save them internally? i would expect S7 (no multiplexor) and S0_m to be always the first values as you would need s0_2s values for all others

No double parsing, no particular order is expected. Basically the muxer publishes its value after its muxed, this is why it appears after. Anyway, no particular order should be expected.

A

[Uight]

@Adhara3 this now should be pretty much it from a parsing perspective. i dint bother to update the tests as i have to rebase anyway but i think this would be an good option. The only thing that needs to be done when convertig to the final Multiplexing Info object is setting the multiplexor for all signals that dont have it set in simple multiplexing.

the role is none if parsing fails but parsing fail is also logged making using flags possible now. Maybe not the nicest that the pasing is not done in the lineparsers but in the pasring multiplex but i think its easy to get that way.

[Uight]

closed for now. ill need to rebase the branch anyway