Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Battery Thermistor Data & Heater Control #196

Closed
10 of 12 tasks
zCoCo opened this issue Dec 29, 2022 · 13 comments · Fixed by #197
Closed
10 of 12 tasks

Fix Battery Thermistor Data & Heater Control #196

zCoCo opened this issue Dec 29, 2022 · 13 comments · Fixed by #197
Assignees
Labels
bug Something isn't working bug-impactful Bug with the potential to degrade mission objectives. _CRITICAL Very important. Need this working ASAP. enhancement New feature or request GSW-GDS Ground Data System WD Code changes to the WatchDog
Milestone

Comments

@zCoCo
Copy link
Member

zCoCo commented Dec 29, 2022

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:

  • Poor Telemetry Packing
  • Unable to Read Thermistors
  • Bad Heater Control

FIxes Required:

  • Determine if this also occurs on devkit (i.e. could it be just a packing / FSW issue or is HW definitely a component -- note: we can't definitively rule out HW w/out access to FM1. The most we can do is determine if FSW is also a problem).
  • Allow charging thermistor to be used in KA and drive the thermistor measurement (PU only in KA and only if REGE is off).
  • Enable using battery thermistor, charging thermistor, or fusion of both as temperature source.
  • Audit and potentially revise heater control algorithm. Make it a basic state machine.
  • Make heater control able to be entirely overridden by ground commands (so we can just bang-bang the heater from ground if needed - e.g. based on deck thermistor or something).
  • Make Heater Control Settings Persistent #200
  • Fix: Heater is always being commanded on a 0% (PWM is defaulting to 0)
  • Fix: Max PWM is low.
  • Make some test command aliases that set various thresholds and powers.
@zCoCo zCoCo added bug Something isn't working enhancement New feature or request _CRITICAL Very important. Need this working ASAP. GSW-GDS Ground Data System WD Code changes to the WatchDog bug-impactful Bug with the potential to degrade mission objectives. labels Dec 29, 2022
@zCoCo zCoCo added this to the RC7 milestone Dec 29, 2022
@zCoCo zCoCo self-assigned this Dec 29, 2022
@zCoCo
Copy link
Member Author

zCoCo commented Dec 30, 2022

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
Copy link
Member Author

zCoCo commented Dec 30, 2022

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:

  • Heater is being bang-banged rn but always at 0% power when "on"
  • OR Heater is being bang-banged rn from 0% to 100% and the PWM is disabled somehow (TBD), in which case PWM code should be a.) deprecated and removed from telem OR b.) reinstated to dictate the "on" heater power (preferable).

@zCoCo
Copy link
Member Author

zCoCo commented Dec 30, 2022

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
Copy link
Member Author

zCoCo commented Dec 30, 2022

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
Copy link
Member Author

zCoCo commented Dec 30, 2022

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

@zCoCo
Copy link
Member Author

zCoCo commented Dec 31, 2022

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
Copy link
Member Author

zCoCo commented Jan 1, 2023

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

@zCoCo
Copy link
Member Author

zCoCo commented Jan 1, 2023

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 zCoCo linked a pull request Jan 11, 2023 that will close this issue
@zCoCo
Copy link
Member Author

zCoCo commented Jan 11, 2023

Some features still need to be impl.

@zCoCo zCoCo reopened this Jan 11, 2023
@zCoCo
Copy link
Member Author

zCoCo commented Jan 11, 2023

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
Copy link
Member Author

zCoCo commented Mar 1, 2023

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
Copy link
Member Author

zCoCo commented Mar 1, 2023

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

@zCoCo zCoCo closed this as completed Mar 1, 2023
@zCoCo zCoCo changed the title WD on FM1 Doesn't Appear to have Battery Thermistor Data. Fix Battery Thermistor Data & Heater Control Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bug-impactful Bug with the potential to degrade mission objectives. _CRITICAL Very important. Need this working ASAP. enhancement New feature or request GSW-GDS Ground Data System WD Code changes to the WatchDog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant