openpilot
openpilot copied to clipboard
commaai
tici fan speed affected by voltage changes
Describe the bug
On vehicles with smart alternator control (mild hybrid, recuperation coasting) and/or automatic engine start-stop features, the +12V nominal supply can vary anywhere from high 11s to low 14s, and do so quite suddenly. For C3, this ends up changing the fan speed enough to be audibly noticeable, and mildly annoying in city driving.
This was discussed in EPHOT, and I could swear it was fixed last summer, but now I can't find evidence of that. As far as I can see, thermald outputs a desired fan speed percentage based on temperature, which gets used by Dos as a PWM width without any control feedback from voltage or fan RPM.
Provide a route where the issue occurs
3cfdec54aa035f3f|2022-05-31--09-26-57 segments 158 through 160
openpilot version
master @ b5aed2bf6781cd4d74d28f6f44d24737ec08f0db
Additional info
No response
Don't think there was ever an attempt to fix it, but we definitely have discussed this in the past. What I propose as a fix is to have a very simple controller on fan RPM in panda, especially the D-term should limit the effect you're describing quite a bit.
This should also make sure there's a better mapping between desired speed and actual speed when there's differences between devices' fans. Not sure if we want to keep the setpoint as a percentage like now (and have a static mapping between setpoint -> RPM), or have the RPM as the setpoint from openpilot directly
This also requires a change in the way we do the RPM measurement in panda now, since that's probably not fast enough to do any reasonable control with (it just counts the pulses in a given time window).
7679f4fa90d936850113cd345b23313bf25fde6e doesn't directly target this, but should improve the situation a bit. Let us know how it's sounding now. If it's still a problem, should be easier to play with now that all the infra's already setup in the panda for this.
This isn't truly apples::apples, because it's September and I don't have as much solar load to drive up the fan PWM demand. The fan is inaudible over the vehicle HVAC at these low speeds. But, the data do show the new fan controller is doing quite a lot to compensate for engine auto start-stop.
There might be room for improvement, but I wouldn't object if you want to close this until better hot-weather data shows a need.
3cfdec54aa035f3f|2022-09-01--17-50-13
Nice, looks a lot better indeed! If it doesn't turn out to be acceptable as-is in warmer weather, feel free to reopen this and I'll spend some more time on it!
Got a hot sunny drive today, and yeah this does seem like it still needs help.
3cfdec54aa035f3f|2022-09-02--14-37-10
The whole drive:
Just the final 20% of the drive:
The voltage-driven fan speed excursions get larger as fan speed increases. In the city driving at the end, the frequent 600-800 RPM excursions really audibly stand out as I'm in-and-out of coast/regen in addition to auto start-stop.
Intuitively this seems fixable by considering voltage directly in Panda's feedforward calculation. Once a PWM/voltage curve is plotted out on a baseline indoor C3, the only remaining error to clean up should be from air density (altitude, temperature) and perhaps age of the fan.
Looks like Github won't let me reopen an issue closed by comma, so it's up to you whether you want to reopen this one or whether I should create a new one.
Ok, will re-open this and look into it again once there's some spare time.
I thought about doing feedforward with the car voltage, but the issue is that we don't really have a way to measure this panda-side on the C3 (so we would have to have boardd forward this, which doesn't seem very clean).
I'll think about it a little more, maybe there is a cleaner solution!
I thought about doing feedforward with the car voltage, but the issue is that we don't really have a way to measure this panda-side on the C3 (so we would have to have boardd forward this, which doesn't seem very clean).
Hah, I was wondering why you didn't use it to begin with. I thought the entirety of peripheralState came from Panda, I see now where it's created in boardd. Guess we can't use ignition voltage either due to CAN ignition cars.
I'll think about it a little more, maybe there is a cleaner solution!
:+1: