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

Implement Timer based on ESP-IDF API #5931

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Implement Timer based on ESP-IDF API #5931

merged 1 commit into from
Dec 14, 2021

Conversation

P-R-O-C-H-Y
Copy link
Member

Summary

This PR is refactoring of TIMER HAL in order to use IDF instead of current Register manipulation approach.

I will do more tests, but everything seems to be working on all chips.

Impact

None.

@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this Nov 26, 2021
@P-R-O-C-H-Y P-R-O-C-H-Y linked an issue Nov 26, 2021 that may be closed by this pull request
3 tasks
@PaintYourDragon
Copy link
Contributor

PaintYourDragon commented Nov 26, 2021

Noticed that the RepeatTimer example (main branch) doesn’t work on ESP32-S2 (works OK on ESP-WROOM-32). Before I open a new issue…looks like it might be related & addressed by this PR? If so, will just keep an eye on the status here. Thx.

@PaintYourDragon
Copy link
Contributor

Answering my own question (and for author’s and maintainers’ edification): can confirm this fixes the RepeatTimer example, correctly runs on both WROOM and S2 after this. Looks like it gets Adafruit_Protomatter working again as well. Thx!

@PaintYourDragon
Copy link
Contributor

Tagging @ladyada on this to follow along on current status.

@me-no-dev me-no-dev merged commit 5e7db8d into espressif:master Dec 14, 2021
@VojtechBartoska VojtechBartoska added this to the 2.0.2 milestone Dec 15, 2021
@AkiHigashi
Copy link

Timer1 and Timer2 can't be used at the same time since 2.0.2. I believe this is because...

// Works for all chips
static hw_timer_t timer_dev[4] = {
{0,0}, {1,0}, {1,0}, {1,1}
};

right ?

@P-R-O-C-H-Y
Copy link
Member Author

P-R-O-C-H-Y commented May 9, 2022

Hi @AkiHigashi, All timers work at the same time. Tested on 2.0.3 :)

// Works for all chips
static hw_timer_t timer_dev[4] = {
{0,0}, {1,0}, {1,0}, {1,1}
};

This is just definition of timer groups / numbers for ESP-IDF API.

@AkiHigashi
Copy link

In the function
hw_timer_t * timerBegin(uint8_t num, uint16_t divider, bool countUp)

timer_dev[] is used as
hw_timer_t * timer = &timer_dev[num]; //Get Timer group/num from 0-3 number

So I think in the cases num=1 and num=2 result in the same [hw_timer_t * timer].

How?

@P-R-O-C-H-Y
Copy link
Member Author

P-R-O-C-H-Y commented May 9, 2022

typedef struct hw_timer_s
{
    uint8_t group;
    uint8_t num;
} hw_timer_t;

// Works for all chips
static hw_timer_t timer_dev[4] = {
    {0,0}, {1,0},  {1,0},  {1,1}
};

The 1st number is GROUP, 2nd number is TIMER NUMBER.

timer_dev[] is used as
hw_timer_t * timer = &timer_dev[num]; //Get Timer group/num from 0-3 number

Read the comment on that line, it will get the combination of group/number from the num passed to the begin function.

Then it will configure the timers by group/num.

timer_init(timer->group, timer->num, &config);

If you call timerBegin() with 0 / 1 / 2 or 3 you will always get different group/timer_num :)

You can check arduino-esp32 timer doc and ESP-IDF timer docs if needed.

@AkiHigashi
Copy link

Last comment.

timerBegin(0, uint16_t divider, bool countUp)
hw_timer_t * timer = &timer_dev[num] ------> timer->group=0; timer->num=0;

timerBegin(1, uint16_t divider, bool countUp)
hw_timer_t * timer = &timer_dev[num] ------> timer->group=1; timer->num=0;

timerBegin(2, uint16_t divider, bool countUp)
hw_timer_t * timer = &timer_dev[num] ------> timer->group=1; timer->num=0;

timerBegin(3, uint16_t divider, bool countUp)
hw_timer_t * timer = &timer_dev[num] ------> timer->group=1; timer->num=1;

static hw_timer_t timer_dev[4] = {
{0,0}, {1,0}, {1,0}, {1,1}
};

should be

static hw_timer_t timer_dev[4] = {
{0,0}, {0,1}, {1,0}, {1,1}
};

In esp32-hal-timer.c (2.0.1)

typedef struct hw_timer_s {
hw_timer_reg_t * dev;
uint8_t num;
uint8_t group;
uint8_t timer;
portMUX_TYPE lock;
} hw_timer_t;

static hw_timer_t hw_timer[4] = {
{(hw_timer_reg_t *)(DR_REG_TIMERGROUP0_BASE),0,0,0,portMUX_INITIALIZER_UNLOCKED},
{(hw_timer_reg_t *)(DR_REG_TIMERGROUP0_BASE + 0x0024),1,**0,1,**portMUX_INITIALIZER_UNLOCKED},
{(hw_timer_reg_t *)(DR_REG_TIMERGROUP0_BASE + 0x1000),2,1,0,portMUX_INITIALIZER_UNLOCKED},
{(hw_timer_reg_t *)(DR_REG_TIMERGROUP0_BASE + 0x1024),3,1,1,portMUX_INITIALIZER_UNLOCKED}
};

bye

@P-R-O-C-H-Y
Copy link
Member Author

Yep you are right, sorry i was blind. That is a mistake in the definition. Should be like you posted. Thanks for finding that. I will make a PR with a fix :)

@AkiHigashi
Copy link

Tnx!

@mrengineer7777
Copy link
Collaborator

@P-R-O-C-H-Y The PR changes the definition of static hw_timer_t timer_dev[4] but the bytes are swapped compared to what @AkiHigashi posted. Impact: Timers 1 & 2 might be swapped, but they will be unique.

@P-R-O-C-H-Y
Copy link
Member Author

@P-R-O-C-H-Y The PR changes the definition of static hw_timer_t timer_dev[4] but the bytes are swapped compared to what @AkiHigashi posted. Impact: Timers 1 & 2 might be swapped, but they will be unique.

@mrengineer7777
They need to be swaped, because esp32-C3 has 2 timer groups with 1 timer in each group. So you have availible timer 0 and 1. If swaped, there will be timer 0 and 2 which makes no sense ;)

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.

Change Timer to use ESP-IDF API
6 participants