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

Change timing scheduler to only run main timer once ADC is done #1535

Merged
merged 6 commits into from
Jan 30, 2023

Conversation

Ralim
Copy link
Owner

@Ralim Ralim commented Jan 23, 2023

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes

Changes from using a fixed timebase for the control of the main heater to instead waiting for ADC to finish.
This means if IRQ's are delayed (during I2C for example) it wont over-run into the next time slot and cause ADC to mis-read.

@Ralim
Copy link
Owner Author

Ralim commented Jan 23, 2023

@joric
@river-b
@cohaolain

Heads up; would love some testing on this if any of you get a chance

@Ralim Ralim added this to the 2.21 milestone Jan 23, 2023
@cohaolain
Copy link
Contributor

@joric @river-b @cohaolain

Heads up; would love some testing on this if any of you get a chance

Just tested this now - still getting those spikes unfortunately, so doesn't look like this has fixed it.

@joric
Copy link

joric commented Jan 23, 2023

Yep still the same. I've made sure I rebuilt everything. did git fetch upstream. git checkout testing-fix-for-timer-slot-miss. Firmware version is 2.20.5FE019F 23-01-23. Used 50C base temp and QC charger at 9V. Still randomly spikes to 473 on display.

@Ralim
Copy link
Owner Author

Ralim commented Jan 24, 2023

Hmm damn.
I did push one extra little tweak; test the new CI builds (prefer CI to ensure we are all testing the exact same) to see if it fixes it for you.
I havent been able to replicate this here one the unit I normally carry with me.
(At least not visible to me at 9V,20V or 28V tests).
Any hints on reproducing it reliably for your devices?

@joric
Copy link

joric commented Jan 24, 2023

Same. Looks like it's more about NOT reproducing it reliably. Tried switching the PSU, it's the same this one is Baseus 65W GAN PD at 20V. Stock 2.18 is not affected.

out.mp4

@Ralim
Copy link
Owner Author

Ralim commented Jan 24, 2023

Attempt 3;
Can you give this one a test?

@joric
Copy link

joric commented Jan 24, 2023

@Ralim that one worked. No more spikes. V2.20.960382C 24-01-23. Used artefacts from here https://github.com/Ralim/IronOS/actions/runs/3994100685

@River-Mochi
Copy link
Contributor

does this still need testing? looks like it's all good now?

@cohaolain
Copy link
Contributor

cohaolain commented Jan 24, 2023

Tested for a couple of minutes just now, latest commit seems to have eliminated the temp spikes 👍

PID might need a little re-tuning though if the heater timings have changed significantly - in my case, seems to be consistently overshooting the target temp by a few degrees with these changes.

But looks like the primary issue is resolved 🎊

@Ralim
Copy link
Owner Author

Ralim commented Jan 30, 2023

Excelllent; thanks for testing 🤗

Going to merge this in, then look at PID.

While the Timings shouldn't have changed much They will have so will re-run testing.

@Ralim Ralim merged commit bb76dfe into dev Jan 30, 2023
@Ralim Ralim deleted the testing-fix-for-timer-slot-miss branch January 30, 2023 06:33
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.

4 participants