[Ovmsdev] Leaf 2016 30Kwh flapping SOC

[Tom Parker]

On 03/05/18 21:25, Robin O'Leary wrote:

> On Wed, May 02, 2018 at 11:55:19PM +1200, Tom Parker wrote:
>> My notes indicate that 0x55b on the EV bus contains the state of charge, and
>> I've implemented this at https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/compare/master...carrott:nissan-leaf-soc-instrument?expand=1
>> along with a configuration option to choose how to show the SOC.
> Comments on this:
>
> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/compare/master.
>
> A purely stylistic point: Many Leaf CAN packets contain several values,
> some of which may be valid while others have an "undefined" value
> (typically all-1s).  So I think it's better not to "break" out of the
> case this:
>
> +      uint16_t soc10 = ((uint16_t) d[0] << 2) | ((d[1] & 0xc0) >> 6);
> +      if (soc10 == 1023)
> +        {
> +        // ignore invalid data seen during startup
> +        break;
> +        }
> ...

Good point. I've re-factored the 0x5bc instance of this pattern and 
pushed to the same branch. I also grabbed the instrument SOC from 0x1db 
and swapped the default source of the main SOC metric. What do you think?

This is displaying the correct instrument cluster soc for me.

I also found that I couldn't make the code return to using the 
instrument SOC return false after populating the config value, setting 
the config value to any of the not-true values or even removing it 
altogether didn't make it go back. I haven't looked at what is wrong yet.

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. I'm not sure if 
this would actually be better without trying it, but the big switch 
certainly feels wrong, not lest because I'm creating blocks inside the 
case statements and using local variables not visible to the other cases!

> Instead of creating a new option, could we not use whether or not the
> existing "newCarAh" option is set?  It needs to be set to a correct
> value in order for the old method to work, and people who don't set it
> would just get the dash version.

I think we can discover the battery size from the SOH, CAC, SOC and GIDs 
metrics, so we can get rid of the newCarAh and newCarGIDS metrics 
altogether.

There may be an explicit signal on the CAN bus which has the new car 
battery size?