-
-
Notifications
You must be signed in to change notification settings - Fork 19.5k
🩹 Add unmeasured ADC reference warning for MKS_TINYBEE board (#27434) #27755
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
🩹 Add unmeasured ADC reference warning for MKS_TINYBEE board (#27434) #27755
Conversation
dda7d19
to
fda62f8
Compare
*/ | ||
|
||
#ifndef ADC_REFERENCE_VOLTAGE | ||
#warning "If you experience odd temperature readings make sure to measure the analog reference voltage. See pins_MKS_TINYBEE.h for details" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea, but you get a wall of warnings due to the many (re)includes of board pin files: https://github.com/MarlinFirmware/Marlin/actions/runs/14003259323/job/39213443240?pr=27755#step:8:49
There are a couple options:
- Add this warning via
warnings.cpp
if this motherobard is enabled - Implement something like what was done for the
CONTROLLER_WARNING
macro if you want to halt compilation and force the user to measure their board / set the voltage / disable the warning:Lines 126 to 130 in 14fa705
#ifndef NO_CONTROLLER_CUSTOM_WIRING_WARNING #define CONTROLLER_WARNING(PF,CN,V...) static_assert(false, "\n\nWARNING! " CN " requires wiring modification! See pins_" PF ".h for details." V "\n (Define NO_CONTROLLER_CUSTOM_WIRING_WARNING to suppress this warning.)\n\n"); #else #define CONTROLLER_WARNING(...) #endif
It kind of depends on just how dodgy that part is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback!
I don't have any other sample besides my own experience and the linked issues, so maybe it's not that common? I don't really know... Could it pose a security hazard? If no temperature safeguards are enabled it could be...? I might be splitting hairs here...
I think having one warning would be enough to make users aware of the problem. In the end, having a 30C difference is quite noticeable 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just move the warning, but I don't know what the proper/expected way to do it in this codebase because something that defines ADC_REFERENCE_VOLTAGE
that is being included in Warnings.cpp so the warning is not triggered properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ADC_REFERENCE_VOLTAGE
is defined in the pins file / via an #ifndef, so you can simplify the #if
check in the warning:#if MB(MKS_TINYBEE)
I'd change the warning too.
Combined changes look something like:
/**
* MKS_TINYBEE Analog Reference
*/
#if MB(MKS_TINYBEE)
#warning "If you experience odd temperature readings make sure to measure the analog reference voltage and add '#define ADC_REFERENCE_VOLTAGE <value>' to your config. See pins_MKS_TINYBEE.h for details."
#endif
4354891
to
efa1758
Compare
Description
MKS_TINYBEE uses a voltage reference based on a resistor-zener circuit. In some boards the zener is of dubious quality and it leads to a poor voltage reference, which in turn throws the voltage readings off (in my case, 30°C). This PR lets the user know of that with a compiler warning, so that it can be dealt with accordingly.
This follows up on #24142 and #24432, and closes #27434.
The reference circuit, out of the manufacturer's schematic:

Requirements
MKS_TINYBEE with a different voltage reference.
Benefits
It lets the user know of a common case, which can be slightly hard to track down. It allows for the related constant to be defined easily.
Configurations
Default MKS_TINYBEE.
Related Issues
#24142
#27434