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

Monitor all temperature sensors #922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kimble4
Copy link
Contributor

@kimble4 kimble4 commented Oct 25, 2024

Sets _temps[EVSE_MONITOR_TEMP_MONITOR] and _temps[EVSE_MONITOR_TEMP_MAX] using the temperature sensor with the highest reading, rather than always using the first sensor with a valid reading.

Sets `_temps[EVSE_MONITOR_TEMP_MONITOR]` and `_temps[EVSE_MONITOR_TEMP_MAX]` using the temperature sensor with the highest reading, rather than always using the first sensor with a valid reading.
@jeremypoulter
Copy link
Collaborator

Thanks for your contribution, however EVSE_MONITOR_TEMP_MAX was already the max temp, I am not sure EVSE_MONITOR_TEMP_MONITOR should be the same. EVSE_MONITOR_TEMP_MONITOR is the temp sensor that will most likely be monitoring the temp of the cable/connector to the EV that so that the current can be limited if it gets too hot. This is no nessecerially the hotest temp sensor, eg you don't what your charging current limited or cut off becuse the WiFi board is doing some number crunching and gets a little hot, but in most cases it won't matter as there will only be one temp sensor. Maybe you can describe your use csae.

@kimble4
Copy link
Contributor Author

kimble4 commented Oct 27, 2024

It appeared to be a bug:

EVSE_MONITOR_TEMP_MAX is presumably intended to be the peak temperature seen since reboot. It isn't reset when getTemperatureFromEvse() is called, so only ever gets increased as higher temperatures are recorded. That makes sense; peak temperature is a useful thing to track.

Maybe I misunderstand what EVSE_MONITOR_TEMP_MONITOR is for? It's reported in the web GUI as "Main T°", and over MQTT as temp (the API docs describe it as "the derived temperature") and is what I used for the TFT temperature display in #914. I assume therefore that it's supposed to be "the EVSE's current temperature". If only a single temperature sensor is installed (eg. the current kit ships with a MCP9808 as its only temperature source), then it tracks the latest value from that sensor, which is reasonable.

As I also have a DS3232 fitted (because I wanted a real-time clock) as well as a MCP9808, I noticed that EVSE_MONITOR_TEMP_MONITOR was always reporting the temperature from the DS3232, even when the MCP9808's temperature was higher. Testing with a hot air gun, I confirmed this lead to a confusing situation where the OpenEVSE could (correctly) trip out with an over-temperature condition due to the MCP9808, but the "Main T°" would still display the lower temperature from the DS3232.

Examining the code, I saw that EVSE_MONITOR_TEMP_MONITOR is invalidated when getTemperatureFromEvse() is called, and then the available temperature sensors are iterated over in numerical order until a valid reading is found. It then breaks out of the loop without trying the other sensors. This means you always get the DS3232's temperature, unless it's not valid, in which case it tries the MCP9808, and so on. (This value is then used to increase EVSE_MONITOR_TEMP_MAX if appropriate, so that records the highest temperature seen since reboot, but only by the first valid sensor.)

This seems wrong, if the intention is that EVSE_MONITOR_TEMP_MONITOR is the aggregate EVSE temperature. My fix makes EVSE_MONITOR_TEMP_MONITOR the highest of all available sensor readings now, and EVSE_MONITOR_TEMP_MAX the highest reading of all available sensor readings since reboot. (Ie. if only one sensor is fitted, the behaviour is unchanged.)

If on the other hand EVSE_MONITOR_TEMP_MONITOR is a separate thing intended to monitor the input connector temperature, why is it being set to the DS3232/MCP9808 temperature by default, and reported as "Main T°"? My understanding is that input connector monitoring is what the TMP007 support (temp3) was originally intended for, but if that were present it would be ignored here in favour of the earlier sensors. As it stands it's more a "temperature of the electronics".

If this change isn't appropriate, may I suggest some comments clarifying what EVSE_MONITOR_TEMP_MONITOR and EVSE_MONITOR_TEMP_MAX are actually for in evse_monitor.h?

(Obviously the OpenEVSE board's handling of its three temperature sensors is independent of all this. It has a separate over-temperature threshold for temp3, for input connector monitoring, but will respond to an over-temperature condition on any of the sensors.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants