lincomatic / open_evse

Firmware for Open EVSE
GNU General Public License v3.0
114 stars 163 forks source link

m_TMP007_temperature stuck at initialization of 230 if not present #62

Closed sandeen closed 7 years ago

sandeen commented 7 years ago

All 3 temp monitors are initialized to 230 (23C):

void TempMonitor::Init()
{
  m_Flags = 0;
  m_MCP9808_temperature = 230;  // 230 means 23.0C  Using an integer to save on floating point library use
  m_DS3231_temperature = 230;   // the DS3231 RTC has a built in temperature sensor
  m_TMP007_temperature = 230;

But if a TMP007 is not present, it stays at 230, which is misleading. This is because although m_tmp007.begin() returns false if it fails, the return value isn't checked.

When we read the temperature:

m_TMP007_temperature = m_tmp007.readObjTempC10();

int16_t Adafruit_TMP007::readObjTempC10(void) {
  int16_t raw = read16(TMP007_TOBJ);

  if (raw & 0x1) return (int16_t)NAN;

we should be getting back NAN, but i'm not sure how that works on assignment. In any case, in my unit with no TMP007, the reading for this non-existent device stays at 230. Maybe the simplest solution would be to just initialize everything to 0?

lincomatic commented 7 years ago

I'm not sure why CraigK decided to initialize everything to initialize everything to 230, but it doesn't really matter. I just tested the firmware in the development branch, and $GP returns 0 when a sensor is missing. Are you using older firmware?

In retrospect, 0 was a bad choice of value for missing sensors, since it's a valid temperature, but I guess 0.00C is unlikely.

sandeen commented 7 years ago

I'm running 3.11.3 - sorry, just got started with this stuff, not running development code in the garage, and don't really have hardware to run development for testing at this point. You said "when a sensor" but I think it matters which sensor - I do get 0 for MCP9808, but not for the TMP007. Anyway - On my 3.11.3 box with no TMP007, I get a reading of 230.

$GP
>$OK 277 0 230
lincomatic commented 7 years ago

Right, 3.11.3 has the bug, and returns 230. All developers should generally be using the dev branch code. It's stable enough to use on a live EVSE. Sorry, the real problem is that it's been way too long since code was moved from the development to the master branch. This is because we lost CraigK, who did very rigorous testing. In the meantime, tons of fixes have been added to the development branch code, and it's been used in quite a lot of production systems. I discussed w/ Chris, and he agrees that it's time to promote the latest dev code to stable.

sandeen commented 7 years ago

Ok, so just so I understand your patch flow, do you ever spot-fix bugs in the release stream, or is it all-or-nothing rebases from development?

What's the best way to do testing on a bench? I don't have an EV simulator and don't really want to sacrifice the one in the garage. ;) Are there any serial commands to do pilot state changes for testing, for example, or would that be possible?

lincomatic commented 7 years ago

All or nothing rebases from development is how we've been doing things. I know, it's not ideal, but that's all I've had time for. It would require too much of a time commitment for me to maintain the branches separately.

The best way to test on a bench is really with an AC powered EVSE and an EVsim. I tried to software simulation before, but it's really impossible to replicate the behavior exactly. Even the EVsim sometimes doesn't catch scenarios that only happen with a live EV. I'll ask Chris to send you an EVsim.

What I often do is #define NOCHECKS when bench testing... this turns off the safety checks, so you don't have to wire up a full blown EVSE to get past them.