GobySoft / dccl

Dynamic Compact Control Language
Other
17 stars 13 forks source link

(unreleased): Exception for resolution() when only precision() field present. #97

Closed psmskelton closed 1 year ago

psmskelton commented 1 year ago

Take a message that has a field:

   required int32 estimated_path_length = 4
   [
     (dccl.field).min = 0,
     (dccl.field).max = 16383,
     (dccl.field).precision = -2
   ];

With the new implementation of (dccl.field).resolution (using the v4.0.2 tag), the stepping between (dccl.field).max and (dccl.field).min must be a multiple of (dccl.field).resolution, down to 1e-6 (IIRC). As evident by this field, this wasn't enforced previously (we've deployed the above message).

However, now an Exception is thrown:

dccl.DcclException: Field estimated_path_length failed validation: (dccl.field).max must be an exact multiple of (dccl.field).resolution

This is misleading as:

  1. The field uses (dccl.field).precision instead of the new (dccl.field).resolution, so debugging for people who do not know about this change would be difficult.
  2. The Exception should state ((dccl.field).max - (dccl.field).min)) must be an exact multiple of (dccl.field).resolution.

EDIT 1: Fixed precision->resolution in 2nd paragraph.

tsaubergine commented 1 year ago

Hi -

Thanks for this. "(dccl.field).max = 16383, (dccl.field).precision = -2" doesn't make a lot of sense as the largest value that could actually be sent would be 16300. However, since it was previously allowed, I will restrict the new check (https://github.com/GobySoft/dccl/pull/98) to messages that explicitly use resolution, instead of precision. Instead I will output a dlog warning for fields that set precision that do not conform to this check.

It's worth noting that when you're on the boundary of a bit, you can actually cost yourself a bit that can't actually be used, which this new check avoids:

--> 9 bits, but max usable value is 25500
required int32 u6 = 8 [
     (dccl.field).min = 0,
     (dccl.field).max = 25599,
     (dccl.field).precision = -2
];

required by new check:

--> 8 bits, max usable value is still 25500
required int32 u6 = 8 [
     (dccl.field).min = 0,
     (dccl.field).max = 25500,
     (dccl.field).precision = -2
];

The Exception wording is correct: the requirement is that max and min be exact multiples of the the resolution, not the difference. For example, as implemented, this is valid:

    required double u3 = 5 [
        (dccl.field).max = 15.5,
        (dccl.field).min = 5.5,
        (dccl.field).resolution = 0.5
    ];

but this is not:

# is not supported: hypothetically would be 5.22, 5.72, 6.22, ... 15.22
    required double u3 = 5 [
        (dccl.field).max = 15.22,
        (dccl.field).min = 5.22,
        (dccl.field).resolution = 0.5
    ];

I made that design choice based I what I thought was the more logical behavior. If there's a lot of interest in supporting the previous example scenario, this can be done in the future without breaking anyone's existing messages.