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 esp8266 timer1 testing #2709

Merged
merged 5 commits into from
Jan 16, 2024
Merged

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Jan 13, 2024

This PR fixes an issue with HostTests which hangs consistently when testing Timer1 (used by HardwareTimer) on the Esp8266.

The problem occurs whilst checking each timer for consistency against system time.
This happens because the test code doesn't set a callback interrupt routine.
It is only a problem on the Esp8266 because the callback routine is set directly on the interrupt vector.
Other architectures use indirection and so do nothing if the callback isn't set.
NB. Doing this with the Esp8266 would increase interrupt latency which would affect PWM timing, etc.

Normal code uses the HardwareTimer class which contains logic to ensure this doesn't happen during setup.

Timer1 has only a 23-bit counter so fails test with /16 divisor as it wraps at around 1.7s (test duration is 2 seconds).
Fixed by restricting duration to timer maximum (less 1ms).

Elapsed tick calculation check also improved by first checking whether timer is an UP or DOWN counter,
and using timer maximum tick value to handle wraps correctly.

This PR also reduces the number of test loops on the Host (from 2000 to 50).

Note: Bug was introduced in #2456. Timer1 needed to be correctly configured (but inactive) to produce correct result for Esp32.

mikee47 added 4 commits January 12, 2024 21:35
…time.

The is because a callback interrupt routine hasn't been set on the timer.
This doesn't affect normal code which accesses the timer via HardwareTimer which contains
logic to ensure this doesn't happen.
This is because it's only a 23-bit timer which limits maximum to about 1.7 seconds (test duration is 2 seconds).
Fixed by restricting duration to timer maximum.
Check whether timer is an UP or DOWN counter
Use timer maximum tick value to ensure counter wraps correctly
Runs more slowly on Host
Copy link

what-the-diff bot commented Jan 13, 2024

PR Summary

  • Control number of iterations in Clocks.cpp
    Added a conditional loop in Clocks.cpp which enables controlling the number of iterations based on the system's host architecture. This functionality enhances the flexibility of the program.

  • Determine clock direction in Clocks.cpp
    Introduced code in Clocks.cpp to evaluate whether a clock is counting up or counting down. This feature provides valuable insights about the clock's behavior and enhances its transparency.

  • Verify timer's correspondence with system clock in Clocks.cpp
    Included code to check if the timer ticks in line with the system clock. This additional layer of clock consistency check helps in ensuring the accuracy of timing operations.

  • Callback function in Timer1ClockTestTemplate
    Implemented a callback function in Timer1ClockTestTemplate. This function allows for better management and execution of tasks in response to certain events, improving the system responsiveness and performance.

@mikee47
Copy link
Contributor Author

mikee47 commented Jan 13, 2024

@slaff Must remember to run HostTests on all chips (esp8266, rp2040, esp32, esp32c3, esp32s2 and esp32s3 if available) to catch stuff like this! Certainly before a major/minor release I think?

@mikee47 mikee47 changed the title Fix esp8266 timer1 testing [WIP] Fix esp8266 timer1 testing Jan 13, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Jan 13, 2024

Looking again at this today I've found an issue with Timer1 on the esp8266, for exactly the reason I gave above.
This code (modified Basic_Blink sample) doesn't work as expected:

#include <SmingCore.h>
#include <HardwareTimer.h>

#define LED_PIN 2

HardwareTimer procTimer;

bool state = true;

void IRAM_ATTR blink()
{
	digitalWrite(LED_PIN, state);
	state = !state;
}

void init()
{
	Serial.begin(COM_SPEED_SERIAL);
	Serial.systemDebugOutput(true);

	debug_i("Welcome");

	pinMode(LED_PIN, OUTPUT);
	procTimer.initializeMs<250>(blink).startOnce();

	auto timer = new SimpleTimer;
	timer->initializeMs<1000>([]() { debug_i("Hello"); });
	timer->start();
}

We expect the LED to toggle once then stop, but instead it keeps flashing at ~1.7s intervals. This is because the timer is still enabled and just keeps wrapping. Confirmed using HardwareTimer1<TIMER_CLKDIV_256> and we get ~27s intervals.

So timer needs to be explicitly disabled in the callback.

Adding this to the driver code is going to add latency, so will need to take a closer look at this. Suggestions welcome!

@slaff
Copy link
Contributor

slaff commented Jan 15, 2024

@mikee47 ping me when this PR is ready.

@mikee47 mikee47 changed the title [WIP] Fix esp8266 timer1 testing Fix esp8266 timer1 testing Jan 15, 2024
@mikee47
Copy link
Contributor Author

mikee47 commented Jan 15, 2024

@slaff OK, done with this PR.

@slaff slaff merged commit 3d86fb3 into SmingHub:develop Jan 16, 2024
46 checks passed
@mikee47 mikee47 deleted the fix/esp8266-timer1-testing branch January 16, 2024 09:01
slaff pushed a commit that referenced this pull request Jan 19, 2024
This PR fixes an issue with `HostTests` which hangs consistently when testing Timer1 (used by HardwareTimer) on the Esp8266.

The problem occurs whilst checking each timer for consistency against system time.
This happens because the test code doesn't set a callback interrupt routine.
It is only a problem on the Esp8266 because the callback routine is set directly on the interrupt vector.
Other architectures use indirection and so do nothing if the callback isn't set.
NB. Doing this with the Esp8266 would increase interrupt latency which would affect PWM timing, etc.

Normal code uses the `HardwareTimer` class which contains logic to ensure this doesn't happen during setup.

Timer1 has only a 23-bit counter so fails test with /16 divisor as it wraps at around 1.7s (test duration is 2 seconds).
Fixed by restricting duration to timer maximum (less 1ms).

Elapsed tick calculation check also improved by first checking whether timer is an UP or DOWN counter,
and using timer maximum tick value to handle wraps correctly.

This PR also reduces the number of test loops on the Host (from 2000 to 50).

Note: Bug was introduced in #2456. Timer1 needed to be correctly configured (but inactive) to produce correct result for Esp32.
@slaff slaff mentioned this pull request Feb 5, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants