From 624986d42315cd685b29cad7cea3d1a9281c48df Mon Sep 17 00:00:00 2001 From: Andy Shaw Date: Thu, 26 Jul 2018 09:59:19 +0100 Subject: [PATCH] Ensure ADC conversion is complete before reading (#11336) The current Marlin implementation relies on a timer interrupt to start the ADC conversion and read it. However in some circumstances the interrupt can be delayed resulting in insufficient time being available for the ADC conversion. This results in a bad reading and false temperature fluctuations. These changes make sure that the conversion is complete (by checking the ADC hardware via the HAL) before reading a value. See: https://github.com/MarlinFirmware/Marlin/issues/11323 --- Marlin/src/HAL/HAL_AVR/HAL.h | 3 +- Marlin/src/HAL/HAL_DUE/HAL.h | 3 +- Marlin/src/HAL/HAL_ESP32/HAL.h | 3 +- Marlin/src/HAL/HAL_LPC1768/HAL.h | 4 +- Marlin/src/HAL/HAL_STM32F1/HAL.h | 3 +- Marlin/src/HAL/HAL_STM32F4/HAL.h | 3 +- Marlin/src/HAL/HAL_STM32F7/HAL.h | 3 +- Marlin/src/HAL/HAL_TEENSY35_36/HAL.h | 3 +- Marlin/src/module/temperature.cpp | 224 +++++++++++++++------------ Marlin/src/module/temperature.h | 2 + 10 files changed, 140 insertions(+), 111 deletions(-) diff --git a/Marlin/src/HAL/HAL_AVR/HAL.h b/Marlin/src/HAL/HAL_AVR/HAL.h index fff12eb709..02aafc0fbd 100644 --- a/Marlin/src/HAL/HAL_AVR/HAL.h +++ b/Marlin/src/HAL/HAL_AVR/HAL.h @@ -345,7 +345,8 @@ inline void HAL_adc_init(void) { #define HAL_START_ADC(pin) ADCSRB = 0; SET_ADMUX_ADCSRA(pin) #endif -#define HAL_READ_ADC ADC +#define HAL_READ_ADC() ADC +#define HAL_ADC_READY() !TEST(ADCSRA, ADSC) #define GET_PIN_MAP_PIN(index) index #define GET_PIN_MAP_INDEX(pin) pin diff --git a/Marlin/src/HAL/HAL_DUE/HAL.h b/Marlin/src/HAL/HAL_DUE/HAL.h index 805acd28d6..c0f8141e5b 100644 --- a/Marlin/src/HAL/HAL_DUE/HAL.h +++ b/Marlin/src/HAL/HAL_DUE/HAL.h @@ -141,7 +141,8 @@ void eeprom_update_block (const void *__src, void *__dst, size_t __n); inline void HAL_adc_init(void) {}//todo #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_result +#define HAL_READ_ADC() HAL_adc_result +#define HAL_ADC_READY() true void HAL_adc_start_conversion(const uint8_t adc_pin); uint16_t HAL_adc_get_result(void); diff --git a/Marlin/src/HAL/HAL_ESP32/HAL.h b/Marlin/src/HAL/HAL_ESP32/HAL.h index c3a3f00955..28e75b332e 100644 --- a/Marlin/src/HAL/HAL_ESP32/HAL.h +++ b/Marlin/src/HAL/HAL_ESP32/HAL.h @@ -109,7 +109,8 @@ void eeprom_update_block (const void *__src, void *__dst, size_t __n); void HAL_adc_init(void); #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_result +#define HAL_READ_ADC() HAL_adc_result +#define HAL_ADC_READY() true void HAL_adc_start_conversion (uint8_t adc_pin); diff --git a/Marlin/src/HAL/HAL_LPC1768/HAL.h b/Marlin/src/HAL/HAL_LPC1768/HAL.h index d11c552e2b..734bc823fa 100644 --- a/Marlin/src/HAL/HAL_LPC1768/HAL.h +++ b/Marlin/src/HAL/HAL_LPC1768/HAL.h @@ -140,11 +140,13 @@ uint8_t spiRec(uint32_t chan); // ADC #define HAL_ANALOG_SELECT(pin) HAL_adc_enable_channel(pin) #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_get_result() +#define HAL_READ_ADC() HAL_adc_get_result() +#define HAL_ADC_READY() HAL_adc_finished() void HAL_adc_init(void); void HAL_adc_enable_channel(int pin); void HAL_adc_start_conversion(const uint8_t adc_pin); uint16_t HAL_adc_get_result(void); +bool HAL_adc_finished(void); #endif // _HAL_LPC1768_H_ diff --git a/Marlin/src/HAL/HAL_STM32F1/HAL.h b/Marlin/src/HAL/HAL_STM32F1/HAL.h index f538e27609..8d459b1dbb 100644 --- a/Marlin/src/HAL/HAL_STM32F1/HAL.h +++ b/Marlin/src/HAL/HAL_STM32F1/HAL.h @@ -224,7 +224,8 @@ void eeprom_update_block (const void *__src, void *__dst, size_t __n); void HAL_adc_init(void); #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_result +#define HAL_READ_ADC() HAL_adc_result +#define HAL_ADC_READY() true void HAL_adc_start_conversion(const uint8_t adc_pin); diff --git a/Marlin/src/HAL/HAL_STM32F4/HAL.h b/Marlin/src/HAL/HAL_STM32F4/HAL.h index 4eca97601e..b5beefbfe1 100644 --- a/Marlin/src/HAL/HAL_STM32F4/HAL.h +++ b/Marlin/src/HAL/HAL_STM32F4/HAL.h @@ -228,7 +228,8 @@ void eeprom_update_block (const void *__src, void *__dst, size_t __n); inline void HAL_adc_init(void) {} #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_result +#define HAL_READ_ADC() HAL_adc_result +#define HAL_ADC_READY() true void HAL_adc_start_conversion(const uint8_t adc_pin); diff --git a/Marlin/src/HAL/HAL_STM32F7/HAL.h b/Marlin/src/HAL/HAL_STM32F7/HAL.h index 834641c230..c594392d88 100644 --- a/Marlin/src/HAL/HAL_STM32F7/HAL.h +++ b/Marlin/src/HAL/HAL_STM32F7/HAL.h @@ -214,7 +214,8 @@ void eeprom_update_block (const void *__src, void *__dst, size_t __n); inline void HAL_adc_init(void) {} #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_result +#define HAL_READ_ADC() HAL_adc_result +#define HAL_ADC_READY() true void HAL_adc_start_conversion(const uint8_t adc_pin); diff --git a/Marlin/src/HAL/HAL_TEENSY35_36/HAL.h b/Marlin/src/HAL/HAL_TEENSY35_36/HAL.h index dca9a1a0ce..6a11bf93c9 100644 --- a/Marlin/src/HAL/HAL_TEENSY35_36/HAL.h +++ b/Marlin/src/HAL/HAL_TEENSY35_36/HAL.h @@ -142,7 +142,8 @@ uint8_t spiRec(uint32_t chan); void HAL_adc_init(); #define HAL_START_ADC(pin) HAL_adc_start_conversion(pin) -#define HAL_READ_ADC HAL_adc_get_result() +#define HAL_READ_ADC() HAL_adc_get_result() +#define HAL_ADC_READY() true #define HAL_ANALOG_SELECT(pin) NOOP; diff --git a/Marlin/src/module/temperature.cpp b/Marlin/src/module/temperature.cpp index 9d420adb2a..c483fd0903 100644 --- a/Marlin/src/module/temperature.cpp +++ b/Marlin/src/module/temperature.cpp @@ -1679,6 +1679,87 @@ void Temperature::set_current_temp_raw() { temp_meas_ready = true; } +void Temperature::readings_ready() { + // Update the raw values if they've been read. Else we could be updating them during reading. + if (!temp_meas_ready) set_current_temp_raw(); + + // Filament Sensor - can be read any time since IIR filtering is used + #if ENABLED(FILAMENT_WIDTH_SENSOR) + current_raw_filwidth = raw_filwidth_value >> 10; // Divide to get to 0-16384 range since we used 1/128 IIR filter approach + #endif + + ZERO(raw_temp_value); + + #if HAS_HEATED_BED + raw_temp_bed_value = 0; + #endif + + #if HAS_TEMP_CHAMBER + raw_temp_chamber_value = 0; + #endif + + #define TEMPDIR(N) ((HEATER_##N##_RAW_LO_TEMP) > (HEATER_##N##_RAW_HI_TEMP) ? -1 : 1) + + int constexpr temp_dir[] = { + #if ENABLED(HEATER_0_USES_MAX6675) + 0 + #else + TEMPDIR(0) + #endif + #if HOTENDS > 1 + , TEMPDIR(1) + #if HOTENDS > 2 + , TEMPDIR(2) + #if HOTENDS > 3 + , TEMPDIR(3) + #if HOTENDS > 4 + , TEMPDIR(4) + #endif // HOTENDS > 4 + #endif // HOTENDS > 3 + #endif // HOTENDS > 2 + #endif // HOTENDS > 1 + }; + + for (uint8_t e = 0; e < COUNT(temp_dir); e++) { + const int16_t tdir = temp_dir[e], rawtemp = current_temperature_raw[e] * tdir; + const bool heater_on = 0 < + #if ENABLED(PIDTEMP) + soft_pwm_amount[e] + #else + target_temperature[e] + #endif + ; + if (rawtemp > maxttemp_raw[e] * tdir && heater_on) max_temp_error(e); + if (rawtemp < minttemp_raw[e] * tdir && !is_preheating(e) && heater_on) { + #ifdef MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED + if (++consecutive_low_temperature_error[e] >= MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED) + #endif + min_temp_error(e); + } + #ifdef MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED + else + consecutive_low_temperature_error[e] = 0; + #endif + } + + #if HAS_HEATED_BED + #if HEATER_BED_RAW_LO_TEMP > HEATER_BED_RAW_HI_TEMP + #define GEBED <= + #else + #define GEBED >= + #endif + const bool bed_on = 0 < + #if ENABLED(PIDTEMPBED) + soft_pwm_amount_bed + #else + target_temperature_bed + #endif + ; + if (current_temperature_bed_raw GEBED bed_maxttemp_raw && bed_on) max_temp_error(-1); + if (bed_minttemp_raw GEBED current_temperature_bed_raw && bed_on) min_temp_error(-1); + #endif +} + /** * Timer 0 is shared with millies so don't change the prescaler. * @@ -1996,6 +2077,12 @@ void Temperature::isr() { * * This gives each ADC 0.9765ms to charge up. */ + #define ACCUMULATE_ADC(var) do{ \ + if (!HAL_ADC_READY()) next_sensor_state = adc_sensor_state; \ + else var += HAL_READ_ADC(); \ + }while(0) + + ADCSensorState next_sensor_state = adc_sensor_state < SensorsReady ? (ADCSensorState)(int(adc_sensor_state) + 1) : StartSampling; switch (adc_sensor_state) { @@ -2005,21 +2092,30 @@ void Temperature::isr() { constexpr int8_t extra_loops = MIN_ADC_ISR_LOOPS - (int8_t)SensorsReady; static uint8_t delay_count = 0; if (extra_loops > 0) { - if (delay_count == 0) delay_count = extra_loops; // Init this delay - if (--delay_count) // While delaying... - adc_sensor_state = (ADCSensorState)(int(SensorsReady) - 1); // retain this state (else, next state will be 0) + if (delay_count == 0) delay_count = extra_loops; // Init this delay + if (--delay_count) // While delaying... + next_sensor_state = SensorsReady; // retain this state (else, next state will be 0) break; } - else - adc_sensor_state = (ADCSensorState)0; // Fall-through to start first sensor now + else { + adc_sensor_state = StartSampling; // Fall-through to start sampling + next_sensor_state = (ADCSensorState)(int(StartSampling) + 1); + } } + case StartSampling: // Start of sampling loops. Do updates/checks. + if (++temp_count >= OVERSAMPLENR) { // 10 * 16 * 1/(16000000/64/256) = 164ms. + temp_count = 0; + readings_ready(); + } + break; + #if HAS_TEMP_ADC_0 case PrepareTemp_0: HAL_START_ADC(TEMP_0_PIN); break; case MeasureTemp_0: - raw_temp_value[0] += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_value[0]); break; #endif @@ -2028,7 +2124,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_BED_PIN); break; case MeasureTemp_BED: - raw_temp_bed_value += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_bed_value); break; #endif @@ -2037,7 +2133,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_CHAMBER_PIN); break; case MeasureTemp_CHAMBER: - raw_temp_chamber_value += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_chamber_value); break; #endif @@ -2046,7 +2142,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_1_PIN); break; case MeasureTemp_1: - raw_temp_value[1] += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_value[1]); break; #endif @@ -2055,7 +2151,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_2_PIN); break; case MeasureTemp_2: - raw_temp_value[2] += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_value[2]); break; #endif @@ -2064,7 +2160,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_3_PIN); break; case MeasureTemp_3: - raw_temp_value[3] += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_value[3]); break; #endif @@ -2073,7 +2169,7 @@ void Temperature::isr() { HAL_START_ADC(TEMP_4_PIN); break; case MeasureTemp_4: - raw_temp_value[4] += HAL_READ_ADC; + ACCUMULATE_ADC(raw_temp_value[4]); break; #endif @@ -2082,9 +2178,11 @@ void Temperature::isr() { HAL_START_ADC(FILWIDTH_PIN); break; case Measure_FILWIDTH: - if (HAL_READ_ADC > 102) { // Make sure ADC is reading > 0.5 volts, otherwise don't read. + if (!HAL_ADC_READY()) + next_sensor_state = adc_sensor_state; // redo this state + else if (HAL_READ_ADC() > 102) { // Make sure ADC is reading > 0.5 volts, otherwise don't read. raw_filwidth_value -= (raw_filwidth_value >> 7); // Subtract 1/128th of the raw_filwidth_value - raw_filwidth_value += ((unsigned long)HAL_READ_ADC << 7); // Add new ADC reading, scaled by 128 + raw_filwidth_value += ((unsigned long)HAL_READ_ADC() << 7); // Add new ADC reading, scaled by 128 } break; #endif @@ -2094,8 +2192,10 @@ void Temperature::isr() { HAL_START_ADC(ADC_KEYPAD_PIN); break; case Measure_ADC_KEY: - if (ADCKey_count < 16) { - raw_ADCKey_value = HAL_READ_ADC; + if (!HAL_ADC_READY()) + next_sensor_state = adc_sensor_state; // redo this state + else if (ADCKey_count < 16) { + raw_ADCKey_value = HAL_READ_ADC(); if (raw_ADCKey_value > 900) { //ADC Key release ADCKey_count = 0; @@ -2113,94 +2213,12 @@ void Temperature::isr() { } // switch(adc_sensor_state) - if (!adc_sensor_state && ++temp_count >= OVERSAMPLENR) { // 10 * 16 * 1/(16000000/64/256) = 164ms. + // Go to the next state + adc_sensor_state = next_sensor_state; - temp_count = 0; - - // Update the raw values if they've been read. Else we could be updating them during reading. - if (!temp_meas_ready) set_current_temp_raw(); - - // Filament Sensor - can be read any time since IIR filtering is used - #if ENABLED(FILAMENT_WIDTH_SENSOR) - current_raw_filwidth = raw_filwidth_value >> 10; // Divide to get to 0-16384 range since we used 1/128 IIR filter approach - #endif - - ZERO(raw_temp_value); - - #if HAS_HEATED_BED - raw_temp_bed_value = 0; - #endif - - #if HAS_TEMP_CHAMBER - raw_temp_chamber_value = 0; - #endif - - #define TEMPDIR(N) ((HEATER_##N##_RAW_LO_TEMP) > (HEATER_##N##_RAW_HI_TEMP) ? -1 : 1) - - int constexpr temp_dir[] = { - #if ENABLED(HEATER_0_USES_MAX6675) - 0 - #else - TEMPDIR(0) - #endif - #if HOTENDS > 1 - , TEMPDIR(1) - #if HOTENDS > 2 - , TEMPDIR(2) - #if HOTENDS > 3 - , TEMPDIR(3) - #if HOTENDS > 4 - , TEMPDIR(4) - #endif // HOTENDS > 4 - #endif // HOTENDS > 3 - #endif // HOTENDS > 2 - #endif // HOTENDS > 1 - }; - - for (uint8_t e = 0; e < COUNT(temp_dir); e++) { - const int16_t tdir = temp_dir[e], rawtemp = current_temperature_raw[e] * tdir; - const bool heater_on = 0 < - #if ENABLED(PIDTEMP) - soft_pwm_amount[e] - #else - target_temperature[e] - #endif - ; - if (rawtemp > maxttemp_raw[e] * tdir && heater_on) max_temp_error(e); - if (rawtemp < minttemp_raw[e] * tdir && !is_preheating(e) && heater_on) { - #ifdef MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED - if (++consecutive_low_temperature_error[e] >= MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED) - #endif - min_temp_error(e); - } - #ifdef MAX_CONSECUTIVE_LOW_TEMPERATURE_ERROR_ALLOWED - else - consecutive_low_temperature_error[e] = 0; - #endif - } - - #if HAS_HEATED_BED - #if HEATER_BED_RAW_LO_TEMP > HEATER_BED_RAW_HI_TEMP - #define GEBED <= - #else - #define GEBED >= - #endif - const bool bed_on = 0 < - #if ENABLED(PIDTEMPBED) - soft_pwm_amount_bed - #else - target_temperature_bed - #endif - ; - if (current_temperature_bed_raw GEBED bed_maxttemp_raw && bed_on) max_temp_error(-1); - if (bed_minttemp_raw GEBED current_temperature_bed_raw && bed_on) min_temp_error(-1); - #endif - - } // temp_count >= OVERSAMPLENR - - // Go to the next state, up to SensorsReady - adc_sensor_state = (ADCSensorState)(int(adc_sensor_state) + 1); - if (adc_sensor_state > SensorsReady) adc_sensor_state = (ADCSensorState)0; + // + // Additional ~1KHz Tasks + // #if ENABLED(BABYSTEPPING) LOOP_XYZ(axis) { diff --git a/Marlin/src/module/temperature.h b/Marlin/src/module/temperature.h index 08c5e508a9..e8b80f97b5 100644 --- a/Marlin/src/module/temperature.h +++ b/Marlin/src/module/temperature.h @@ -52,6 +52,7 @@ * States for ADC reading in the ISR */ enum ADCSensorState : char { + StartSampling, #if HAS_TEMP_ADC_0 PrepareTemp_0, MeasureTemp_0, @@ -328,6 +329,7 @@ class Temperature { /** * Called from the Temperature ISR */ + static void readings_ready(); static void isr(); /**