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

Add real hw_timer support and convert the pwm module to use it #1057

Merged
merged 1 commit into from
Feb 20, 2016

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Feb 15, 2016

This adds the hw_timer.c that was published by espressif. More importantly it adds arbitration between modules that want to use the hw_timer feature.

This also includes a fixed version of driver/pwm.c that makes use of this. I also fixed some bugs in the
pwm module that should make it work more smoothly and more glitch free. The original code had some mysterious locking that would only do anything on a multicore processor. Even the ESP32 only has a single core running user code.

It now does use the NMI handler as that provides more predictable timing -- especially in the presence of network traffic.

There are still (unresolvable) inconsistencies when the duty cycles of two pwm outputs are very close to each other. If the edges need to be within a few microseconds, then one of the edges tends to get moved by a couple of microseconds. With a modulation frequency of 1kHz, then this is much less than 1% error.

There may still be bugs, and I'll have a go at fixing them if anybody can produce a reproducible setup.

@TerryE
Copy link
Collaborator

TerryE commented Feb 16, 2016

Phillip, this is going to clash with the tasking/gpio PR. Do you mind if we merge this first?

@pjsg
Copy link
Member Author

pjsg commented Feb 16, 2016

Ah yes. If I was smarter, I'd have based it on your branch!

TM_EDGE_INT = 0, //edge interrupt
} TIMER_INT_MODE;

static os_param_t the_owner;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't find any initialization of the_owner before evaluating it for the first time. Is it done somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is implicit initialization of statics to zero at start time (this is the .bss section). If you initialize a static, then the initialisation value goes into the .data segment and this needs to be copied into RAM at start time.

In my early days of programming (in the '70s) you learned to squeeze every byte out of the code -- and this habit of mine with regard to not explicitly initialising static variables to zero dates from when I started to use C in the '80s.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, zeroing BSS is part of the (GC) C standards.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's standard behaviour. And whether you leave a static "uninitialised" or explicitly zero-initialised makes no difference for the behaviour (i.e. an explicit zero doesn't suddenly end up in the data_init section).

@pjsg
Copy link
Member Author

pjsg commented Feb 18, 2016

I rebased my branch on the new dev. Actually, there weren't any collisions and it went cleanly. I also verified that it still works!

devsaurus added a commit that referenced this pull request Feb 20, 2016
Add real hw_timer support and convert the pwm module to use it
@devsaurus devsaurus merged commit e82cb97 into nodemcu:dev Feb 20, 2016
@TerryE
Copy link
Collaborator

TerryE commented Feb 20, 2016

Good. We are getting there!

@eku
Copy link
Contributor

eku commented Feb 28, 2016

@pjsg Should this work out of the box by just enabling the PWM module as usual? Nothing has been added to the documentation, so I guess it should But it doesn't. pwm.setup(6,1000,0) resets NodeMCU without any warning.

@pjsg
Copy link
Member Author

pjsg commented Feb 28, 2016

@eku Hmm.... That works for me... Can you provide a complete code sample? Is there any other setup that you need to do to make this happen?

Also, can you get the return value of node.bootreason() (after the restart)?

Thanks

@eku
Copy link
Contributor

eku commented Feb 29, 2016

@pjsg here we go, a fresh build from the HEAD of the dev branch

NodeMCU 1.5.1 build 20160228 powered by Lua 5.1.4 on SDK 1.5.1(e67da894)
lua: cannot open init.lua
> pwm.setup(6,1000,0)
...reboot...
NodeMCU 1.5.1 build 20160228 powered by Lua 5.1.4 on SDK 1.5.1(e67da894)
lua: cannot open init.lua
> =node.bootreason()
2       2       28      1076166482      0       0       0       0

It's a ESP8266-12 with 4M of flash.

@pjsg
Copy link
Member Author

pjsg commented Feb 29, 2016 via email

@eku
Copy link
Contributor

eku commented Feb 29, 2016

Float build. I included my user_config.

map.zip

@pjsg
Copy link
Member Author

pjsg commented Mar 1, 2016

Your build crashed in platform_gpio_mode (in platform.c).

I built the code with your config and did a float build. The size of platform_gpio_mode was the same as your mapfile indicated. The offset inside platform gpio_mode of your crash pointed to a l32i.n instruction (this does a load of 32 bits and it was loading at address 0 -- hence the trap).

The good news is that mine crashed in the same way. It turns out that this will happen if you build without LUA_USE_MODULES_GPIO.

I'm going to submit a bug for this, and do a pull request to fix it. In the short term, you can work around this by adding LUA_USE_MODULES_GPIO.

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.

5 participants