PlanetaryRobotics / IrisRoverPackage

Complete software package for the Iris Lunar Rover (CMU).
GNU General Public License v3.0
14 stars 2 forks source link

Fix Battery Thermistor Data & Heater Control #196

Closed zCoCo closed 1 year ago

zCoCo commented 1 year ago

A review of logs from RC6 shows that the reading reported for the battery thermistor is always 0b1 or 0b0, even when reported in different packet types. This suggests that it's unable to read that thermistor (possibly a fault). Since we won't get an opportunity to test this prior to the pre-TVAC upload, a robust set of heater control patches that can resolve all possible causes needs to be built.

This includes:

FIxes Required:

zCoCo commented 1 year ago

Trace all m_hParams args. Make sure they're all still used or deprecate (incl. in telem). Make sure commands set what they're supposed to.

zCoCo commented 1 year ago

Seems like PWM is still set since TB0CCR0 and TB0CCR2 are still set. Looks like the heater settings are set to and stay at a default of 0/10000 (0%) -- even if the GPIO is on? See if this is the case before changing.

Either:

zCoCo commented 1 year ago

We should remove old telem from telem packets (allowing us to increase frequency later or gain margin). Rn, this appears to be: m_kpHeater, m_pwmLimit, m_heaterSetpoint, and m_heaterWindow. Might want to add an "force on" or "force off" setting.

zCoCo commented 1 year ago

Seems like ADC might somehow be using 2.8V Vref, despite VeRef+ being wired to 3.3V. Maybe cfg'd to be using an internal reference in MCTL?

Or, maybe not ... MSP430FR5994 User Guide shows only internal 1V2, 1V5, 2V5, or external references. RC6 data logs show 2V5 ADC reading as 3132 / 4095 * 3.3 = 2.524V (seems right) BUT the 2V8 ADC reading shows 4073 / 4095 * 3.3 = 3.283V (possibly 2V8 was off and there's an internal pullup?) SO, either the 2V8 line was at 3.3V or with a 3V3 VeRef+, the ADC saturates at 2.8V (?) Neither of these seem quite right, but the first one seems more likely since the HW state was unknown... TBD SBC dev kit testing will reveal more info...

Update: Testing on devkit shows this was a weird quirk. In mission, 2V5 read as 2.5V, and 2V8 read as 3.18V.

zCoCo commented 1 year ago

Probably should replace charge information (deprecated) in heartbeat with thermal info.

zCoCo commented 1 year ago

Plugged a 5k resistor (i.e. 25C or 298K) into RT port. Heating status changed to NOT_HEATING (suggesting that the WD sees a new ADC value) but Tbatt in DetailedStatus stayed at 1, suggesting a data packing issue. BattTemp in Heartbeat and in Hercules telem incorrectly reported charging thermistor instead of main battery thermistor, so it's possible this is just a detailed status data packing issue.

zCoCo commented 1 year ago

NOTE: Open circuit on BattRT reports ~4080 not 0 on disconnect of thermistor (so good support that thermistor isnt' actually disconnected).

zCoCo commented 1 year ago

Heater power level settings work now HOWEVER, if the power level is set to 10% (only level tested below 20%), the voltage measured won't go back up unless the power level is set to at least 70%. (that is, if you set the power level that low then want to go back up to some intermediate value, you must first set it to >=70%). The PWM waveforms never looked weird in this case though, so it's quite possible this was just a quirk of the cheap pokit DMM/OSC used to probe this system. As a rule of thumb, just don't go below 20% power.

zCoCo commented 1 year ago

Some features still need to be impl.

zCoCo commented 1 year ago

Note: voltage ADC readings are probably off but therm are not b/c therm uses strong (~10k) pull-up; whereas, voltage sensors use weak (>~100k) dividers which means the implicit ~~30k pull-up in the MSP430 ADC will have a greater influence on the results.

zCoCo commented 1 year ago

NOTE: During RC7 we discovered that the ADC reference voltage in the conversion table used to generate GSW's thermistor lookup table was wrong. Fixing this made the BattRT thermistor readings accurate to 0.09ºC at ambient.

zCoCo commented 1 year ago

All major concerns here are closed. Spawning some lower priority lingering issues then closing this one:

zCoCo commented 1 year ago

Key issues fixed. All child issues have been broken out. This can now be closed.