[Ovmsdev] generic dissector for can packets

[Robin O'Leary]

On Thu, May 03, 2018 at 10:51:07PM +1200, Tom Parker wrote:
> On 03/05/18 21:25, Robin O'Leary wrote:
> >On Wed, May 02, 2018 at 11:55:19PM +1200, Tom Parker wrote:
> As a broader concern, I don't really like the giant switch statement with
> most parsing inline. I ported the v2 code over with as few changes as
> possible, and in v2 I was a bit paranoid about running out of call stack
> (the pic processor has a fairly shallow hardware stack, though this fear may
> be misplaced as the compiler might not have even used it?). In the v3 code
> we should consider pulling the parsing code into separate functions which
> are invoked by the big switch.

Well, there is something to be said for keeping that code path as simple
as possible, as it will be run hundreds of times a second---and other
vehicles also take the same approach with a big switch().

And to be honest, I don't really see any benefit if we just end up with:
    ...
    case 0x54c: parse_54c(d); break;
    case 0x5bc: parse_5bc(d); break;
    ...
If we are looking to re-factor properly and reduce repeated code, I'd
abstract out the common bit-manipulation, test for exceptional value
and offset/scale calculations.  Many (most?) cases could be covered with
these parameters in a table:

{ address, bit_shift, mask, ignore_value, offset, scale, metric_object }

e.g.

    case 0x54c:
      /* Ambient temperature.  This one has half-degree C resolution,
       * and seems to stay within a degree or two of the "eyebrow" temp display.
       * App label: AMBIENT
       */
      if (d[6] != 0xff)
        {
        StandardMetrics.ms_v_env_temp->SetValue(d[6] / 2.0 - 40);
        }

Treating d[0..7] as a 64-bit value, d[6] would be >> 8 & 0xff,
so this would be expressed in the table as:

// address shift mask ignore offset scale metric
{ 0x54c,   8,    0xff, 0xff, 80,    0.5,  StandardMetrics.ms_v_env_temp }


This could also handle some more complicated cases with an intermediate
metric, e.g.

    case 0x5bc:
    {
      uint16_t nl_gids = ((uint16_t) d[0] << 2) | ((d[1] & 0xc0) >> 6);
      if (nl_gids == 1023)
        {
        // ignore invalid data seen during startup
        break;
        }
      float km_per_kwh = MyConfig.GetParamValueFloat("xnl", "kmPerKWh", GEN_1_KM_PER_KWH);
      float wh_per_gid = MyConfig.GetParamValueFloat("xnl", "whPerGid", GEN_1_WH_PER_GID);

      m_gids->SetValue(nl_gids);
      StandardMetrics.ms_v_bat_soc->SetValue((nl_gids * 100.0) / max_gids);
      StandardMetrics.ms_v_bat_range_ideal->SetValue((nl_gids * wh_per_gid * km_per_kwh) / 1000);
      break;
    }

The GID value itself fits the scheme:

// address shift mask ignore offset scale metric
{ 0x5bc,   66,   1023, 1023, 0,     1,    m_gids }

The SetValue method of m_gids would then be responsible for dealing with
the other metrics depending on its value.


It seems like this could benefit developers of other vehicles too,
so maybe we should have this conversation with a wider audience.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20180503/5a015166/attachment-0002.sig>