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.