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

Fix counter rising edges when CounterDebounceLow/High is set #20712

Closed
wants to merge 1 commit into from

Conversation

curzon01
Copy link
Contributor

Description:

Counters were only set on falling edges, regardless of whether CounterDebounceLow or CounterDebounceHigh was set or not.

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.6
  • The code change is tested and works with Tasmota core ESP32 V.2.0.14
  • I accept the CLA.

@arendst
Copy link
Owner

arendst commented Feb 13, 2024

DO you have a use-case for this? I ask because there never was a reason to change this code part as far as I know and the impact is not clear to me.

@curzon01
Copy link
Contributor Author

curzon01 commented Feb 13, 2024

DO you have a use-case for this?

Tipping bucket rain gauge:
Each tilting movement closes or opens a reed contact, i.e. each tilting movement corresponds to a fixed amount of rain, but you have to count both the rising and the falling edge

I ask because there never was a reason to change this code part as far as I know and the impact is not clear to me.

I wonder why the counter does not worked as described for CounterDebounceLow/High, maybe I just misunderstand the descritiopn for CounterDebounceLow/CounterDebounceHigh:

"When unequal zero tasmota will check falling and rising edges on the counter's GPIO."

I just wonder what CounterDebounceLow/High exists for if you can't use it to recognize both edges separately.

In any case, with the current implementation it is not possible to count rising and falling edges, that's why I made this PR

@curzon01
Copy link
Contributor Author

Theo, as I read the origin PR (#8021) this PR will not be backward compatible, I think the misleading part is the description for CounterDebounceLow/High.

Anyway, I find the separate detection of falling and rising edges useful in cases like this. Should I solve this via an SO or an additional CounterType?

@arendst
Copy link
Owner

arendst commented Feb 14, 2024

Looking at the important part of the code:

      Counter.any_counter = true;
      pinMode(Pin(GPIO_CNTR1, i), bitRead(Counter.no_pullup, i) ? INPUT : INPUT_PULLUP);
      if ((0 == Settings->pulse_counter_debounce_low) && (0 == Settings->pulse_counter_debounce_high) && !Settings->flag4.zerocross_dimmer) {
        Counter.pin_state = 0;
        attachInterruptArg(Pin(GPIO_CNTR1, i), CounterIsrArg, &ctr_index[i], FALLING);
      } else {
        Counter.pin_state = 0x8f;
        attachInterruptArg(Pin(GPIO_CNTR1, i), CounterIsrArg, &ctr_index[i], CHANGE);
      }

wouldn't it make sense in your case to use PCD_low and PCD_high to use the original CHANGE interrupt?

@curzon01
Copy link
Contributor Author

curzon01 commented Feb 14, 2024

I saw this, this only enables IRQ on both changes but within IRQ routine rising edges are filtered:

    // do not count on rising edge
    if bitRead(Counter.pin_state, index) {
      // PWMfrequency 100
      // restart PWM each second (german 50Hz has to up to 0.01% deviation)
      // restart initiated by setting Counter.startReSync = true;
#ifdef USE_AC_ZERO_CROSS_DIMMER
      if (index == 3) ACDimmerZeroCross(time);
#endif //USE_AC_ZERO_CROSS_DIMMER
      return;
    }

this code part always returns for rising edged before counter is counted

@arendst
Copy link
Owner

arendst commented Feb 14, 2024

Would this solve your issue:

    // do not count on rising edge
    if bitRead(Counter.pin_state, index) {
      // PWMfrequency 100
      // restart PWM each second (german 50Hz has to up to 0.01% deviation)
      // restart initiated by setting Counter.startReSync = true;
#ifdef USE_AC_ZERO_CROSS_DIMMER
      if (index == 3) { ACDimmerZeroCross(time); }
      return;
#else
      if (!Settings->flag6.counter_both_edges) {
        return;
      }
#endif //USE_AC_ZERO_CROSS_DIMMER
    }

making sure the ZeroCross dimmer keeps working.

@curzon01
Copy link
Contributor Author

try to summarize

  • PCD_low/PCD_high already activates IRQ on CHANGE
  • PCD_low/PCD_high was originally intended by the author only to add additonal pre-debounces for falling and rising edges, not to count both separately.
  • PCD_low/PCD_high cmnd description is misleading in that both edges are counted

I want to be backward compatible not touching the current behaviour but allow additonal count of falling & rising edges by an optional setting e.g CounterType 2 or some SO

@curzon01
Copy link
Contributor Author

Would this solve your issue:

    // do not count on rising edge
    if bitRead(Counter.pin_state, index) {
      // PWMfrequency 100
      // restart PWM each second (german 50Hz has to up to 0.01% deviation)
      // restart initiated by setting Counter.startReSync = true;
#ifdef USE_AC_ZERO_CROSS_DIMMER
      if (index == 3) { ACDimmerZeroCross(time); }
      return;
#else
      if (!Settings->flag6.counter_both_edges) {
        return;
      }
#endif //USE_AC_ZERO_CROSS_DIMMER
    }

making sure the ZeroCross dimmer keeps working.

Yes, that's in my mind

@arendst
Copy link
Owner

arendst commented Feb 14, 2024

I'll change it now.

arendst added a commit that referenced this pull request Feb 14, 2024
@arendst
Copy link
Owner

arendst commented Feb 14, 2024

Done.

@arendst arendst closed this Feb 14, 2024
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