Checksum support in PyRFLX

[rssen]

1. class AbstractMessage

• Parameter: aspects: Mapping[str, Mapping[Field, Sequence[Expr]]]

2. class MessageValue

• mapping {ChecksumField: Sequence[Fields over which the checksum is computed]}
• set_checksum(name_of_checksum_field, callable_checksum_method)
  • adds the checksum method to the Message Value
• compute_checksum
  • sets/updates the checksum field
  • similar to preset_field but starts at the INITIAL node (preset_fields starts at the current node), checks only if a field is set (not like preset fields if it has a length and first)

3. checksum methods

• parameters
  • Binary representation of the whole message (in whatever type is most suitable)
  • All parameters which were defined in the specification
  • The checksum method must determine the bounds of fields based on its arguments.
• Currently we do not guarantee that indices are grouped as tuples, so there could be also checksum definition like:
  {"Checksum": {"FCS", [First("F1"), First("F4"), Last("F2"), Last("F5")]}}

[treiher]

1. class AbstractMessage

• Dict {ChecksumField: Sequence[Fields over which the checksum is computed]}

The added attribute to AbstractMessage should be generic. Further aspects could be added to a message in the future. So the type of the attribute should be:

aspects: Mapping[Mapping[str, Sequence[Expr]]]

Example: {"Checksum": {"FCS", [First("F1"), Last("F4")]}}

Note that we thought to have a list of expressions as arguments (which could be indices but also other values), not a list of fields (cf. #222).

• compute_checksum

• sets/updates the checksum field

• possible ways to accomplish this

  • save the current graph and update it every time a field is set/changed
  • write a method similar to preset_field that starts at the INITIAL node (preset_fields starts at the current node)
  • change preset fields to start at the INITIAL node

I'm not sure what you are trying to do here. I think we should discuss that (probably it is easier to do this offline with @jklmnn or me).

3. checksum-methods

• parameters

  • List[Bytes]: holds all continuous blocks of bytes -> new block after a field ("hole") that should not go into the computation of the checksum
  • info, where the field are that have to be set to a static default value before computing the checksum (e.g. the checksum field itself)

The parameters of a checksum method should be:

• Binary represenation of the whole message (in whatever type is most suitable)
• All parameters which were defined in the specification

The checksum method must determine the bounds of fields based on its arguments.

Note: Currently we do not guarantee that indices are grouped as tuples, so there could be also checksum definition like:

{"Checksum": {"FCS", [First("F1"), First("F4"), Last("F2"), Last("F5")]}}

[treiher]

Note: Currently we do not guarantee that indices are grouped as tuples, so there could be also checksum definition like:

{"Checksum": {"FCS", [First("F1"), First("F4"), Last("F2"), Last("F5")]}}

In an offline discussion we decided to change the format to make it easier to detect dependent fields. In addition to single arguments we also allow ranges now. Both can be mixed together (cf. #222).

{
    "Checksum":
     {
        "FCS",
        [
            ValueRange(First("F1"), Last("F2")),
            Variable("F4"),
            ValueRange(First("F4"), Last("F5")),
            Length("F2"),
       ]
    }
}

[rssen]

Would Variable("F2") and Length("F2") both express that "Field F2 needs to be included in the checksum calculation" or is there a difference?

[jklmnn]

Well it would make a difference if it has to be set. Variable("F2") means that F2 has to be set. Length("F2") means that only its length has to be known which doesn't require itself to be set but probably another length field that determine its length.

[treiher]

An interesting special case would be also: ValueRange(First("F4"), Sub(First("F6"), Number(1))). Such an expression could be needed, when there are multiple incoming edges to F6 and F6 is the first field which should not be part of the checksum.

AFAIK we have currently no concrete use case for that, so I'm not sure if we want to invest time into it. But probably it is not too hard to detect the scheme "First(Field) - 1".

[treiher]

I think we should restrict the expressions in value ranges to field bounds, i.e. First and Last (maybe with an exception for the First - 1), otherwise it will be hard to determine the dependent fields.

[senier]

I think we should restrict the expressions in value ranges to field bounds, i.e. First and Last (maybe with an exception for the First - 1), otherwise it will be hard to determine the dependent fields.

But then we could even think about only passing field names: ValueRange("F4", "F6") and maybe add a ValueRangeExclusive("F4", "F7") to encode that the second parameter actually means Sub(First("F7"), Number(1)) (better name to be found).

[treiher]

You are right, that would be sufficient. But I'm not sure if we get a understandable specification for that.

Current syntax: (FCS => (F1'First .. F2'Last, F4, F4'First .. F5'Last, F2)
New syntax: (FCS => (F1 .. F2, F4, F4 .. F5, F2)

For me the new syntax is much harder to read.

For your second idea of an exclusive value I don't even have an idea how to represent that.

[senier]

I agree. And I don't have a good idea for the specification either. We should leave it as is, then and restrict the left side of a range to Some_Field'First and the right side of a range to Some_Field'Last or Some_Field'First - 1.

We could still map that to the more restricted objects I suggested above during model creation (ValueRangeExclusive if the right side is of the form Some_Field'First - 1, ValueRange otherwise).

[treiher]

We could still map that to the more restricted objects I suggested above during model creation (ValueRangeExclusive if the right side is of the form Some_Field'First - 1, ValueRange otherwise).

We could, but I see no real advantage in doing this. ValueRangeExclusive(left, right) would be just a short form of ValueRange(left, Sub(right, Number(1)). It would be just a redundant way to express the same thing. There is nothing similar in our expressions yet.

[senier]

We could, but I see no real advantage in doing this. ValueRangeExclusive(left, right) would be just a short form of ValueRange(left, Sub(right, Number(1)). It would be just a redundant way to express the same thing. There is nothing similar in our expressions yet.

Then let's stick with ValueRange and just restrict the arguments.

[treiher]

The checksum support is incomplete: A checksum is never checked during parsing.

[treiher]

I think we have to differentiate two parts of the automatic setting of checksums:

1. Calculating the checksum by a checksum function.
2. Deciding when the checksum needs to be calculated.

I agree that we can even forbid manual setting of a checksum field by using the set method completely (1). As (2) is not executed if skip_verification is set, there needs to be a "set checksum" method to initiate the calculation manually then.

[jklmnn]

Do we actually want to support manually setting a checksum field?

We have to, to support parsing.

And even that could be done by registering a checksum function that does just that. Maybe we only want to allow automatic setting of checksums?

The checksum field has to be handled similarly to other fields to properly support parsing. Or at least we would need a way to detect when a checksum field should appear.

[treiher]

Do we actually want to support manually setting a checksum field?

We have to, to support parsing.

That sounds like a design issue. The behavior of the public interface for setting fields should be independent of the parsing. What are the options? Would that be hard to change?

[jklmnn]

The current implementation enables checksum checks for parsing and for the message generation without verification.

When generating a message (with verification) the checksum can either be set manually, which will disable all checksum checks, or it can be set automatically. In this case the checksum will be recalculated when fields are changed.

When generating a message without verification all fields (including the checksum itself) have to be set in order. The checksum field can contain any value that is then updated when calling update_checksums. This method should only be called on a message without verification enabled.

When parsing a message the parsing will not fail but the message will not be marked as valid if the checksum is incorrect.

The performance of update_checksums and of valid_message with checksums for messages without verification has not yet been tested or improved.

@rssen we lately had a discussion about understanding and commenting code. Could you please try to explain in comments how the checksum setting works. Especially which checks are done (semantically) and why they're done.
Also _is_checksum_settable somehow changes the internal state (_set_checksum never works before calling _is_checksum_settable). I think either the method name should be changed or it shouldn't change internal state.