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

ESP32: DigitalPulse does not work properly #1175

Closed
chevdor opened this issue May 31, 2017 · 51 comments
Closed

ESP32: DigitalPulse does not work properly #1175

chevdor opened this issue May 31, 2017 · 51 comments
Labels
ESP32 This is only a problem on ESP32-based devices

Comments

@chevdor
Copy link

chevdor commented May 31, 2017

I am using the firmware 1.92.73 in the ESP32.
I tested the following code on ESP32:
digitalPulse(27, 1, 100)

and on ESP8266:
digitalPulse(15, 1, 100)

On the ESP8266, as expected, I get a pulse of 100ms.
On the ESP32, the GPIO is set to HIGH forever.

Moreover, while digitalWrite seems to work, digitalPulse(PIN, S, 100) sets the GPIO to HIGH forever, whatever S is (1 or 0).

@wilberforce wilberforce added the ESP32 This is only a problem on ESP32-based devices label Jun 1, 2017
@chevdor
Copy link
Author

chevdor commented Jun 9, 2017

I tested again with 1.92.77 and the issue is still here.

@chevdor
Copy link
Author

chevdor commented Jun 12, 2017

Still seeing the issue in 1.92.89

@chevdor
Copy link
Author

chevdor commented Jun 12, 2017

A bad workaround that may help in the meantime as long as accuracy is no concern.

function digitalPulse(pin, state, duration) {
    digitalWrite(pin, state);
    setTimeout(function() {
        digitalWrite(pin, !state)
    }, duration)
}

@chevdor
Copy link
Author

chevdor commented Jun 12, 2017

This is a critical issue as many modules (ie, the OLED displays such as SSD1306, ...) use a digitalPulse when the reset pin is defined, in order to... well... reset :)

That leads to very weird results when defining the reset pin for instance, which is quite unexpected.

@wilberforce
Copy link
Member

@jumjum123
The esp-idf example is in micro secs:

https://github.com/espressif/esp-idf/blob/81f98c0a772864cbbaa2f270f7a7f6039c16f4c8/examples/peripherals/rmt_nec_tx_rx/main/infrared_nec_main.c#L68-L71

static inline void nec_fill_item_level(rmt_item32_t* item, int high_us, int low_us)
{
    item->level0 = 1;
    item->duration0 = (high_us) / 10 * RMT_TICK_10_US;
    item->level1 = 0;
    item->duration1 = (low_us) / 10 * RMT_TICK_10_US;
}

and in espruino seems to be millseconds:
https://github.com/espruino/Espruino/blob/master/targets/esp32/jshardwarePulse.c#L78-L83

@chevdor

https://github.com/espruino/Espruino/blob/master/targets/esp32/jshardwarePulse.c#L97

if you change:
if(pulsePolarity) setPulseLow(duration);else setPulseHigh(duration);
to
if(pulsePolarity) setPulseLow(duration*1000);else setPulseHigh(duration*1000);

it might work for you.

@jumjum123
Copy link
Contributor

@ wilberforce, take a look to line 385 in jshardware.c
This should already calculate from milli to micro

@wilberforce
Copy link
Member

@jumjum123
Thanks - so that is already factored.

Any ideas why this is not working?

@chevdor
Copy link
Author

chevdor commented Jun 24, 2017

@wilberforce I see milliseconds everywhere.
I tried however various value and it still fails in 1.92.111

@jumjum123
Copy link
Contributor

Looks like duration greater than 32767 are treated as int16 instead of Unit32.
Created an issue in esp32.com to verify

@chevdor
Copy link
Author

chevdor commented Jun 26, 2017

@jumjum123 interesting. I am usually testing with values around 100~1000 though.

@jumjum123
Copy link
Contributor

Following the docu in Neil Kolbans book, in theory the maximum possible time for pulses is around 104ms.
To do this the divider would be set 255 (actual 80), which means 80Mhz divided by 255 and next a time of 32767 of these ticks.
I would suggest to follow Gordons docu here http://www.espruino.com/Reference#l__global_digitalPulse and use setTimeout.
May be we should add a warning if somebody uses big values.

@chevdor
Copy link
Author

chevdor commented Jun 28, 2017

@jumjum123 isn´t 100 < 104 ???
This is just a test anyway. My pulse are much shorter but I test with my eyes and seeing a ms pulse is not easy. What I know is that a 100ms should not turn on a LED forever, especially when the same code works all fine on an ESP8266. Thanks for the link though.

@jumjum123
Copy link
Contributor

Please read my comment, there is an explanation about the divider actually used.
If you want to ge more info read neil Kolbans book.
BTW, checking ms-pulse with eyes, hmmmm sounds strange to me.
Would'nt it be better to use a cheap logic analyzer (saleae for example) ?

@chevdor
Copy link
Author

chevdor commented Jun 28, 2017

@jumjum123 I did read your comment the doc several times. Please read the issue I reported carefully.

Sending is pulse of 100ms is a valid test. When DigitalPulse (pin, 1, 100) sets the output to a high level forever (forever > 100ms ....), this is obviously an issue and no one need a logic analyzer to see it. You are right however, with this method, I don´t know if the pulse is really 100ms and not 105 or 95ms... but this is not the problem here.

That issue does NOT show up on ESP8266.

@jumjum123 are you telling me that you tested on a ESP32 and you can successfully see 100ms pulses (you can use your logic analyzer :))

@jumjum123
Copy link
Contributor

jumjum123 commented Jun 28, 2017

@chevdor, let me try to explain with other words.
Actual version of ESP32 port uses a divider of 80 to get a 1mhz signal, or 1usec. This then is counted internally to given value, which is a signed 16 bit number. Therefore you can go up to about 32msecs.
Extending this to 100msec would need to change the divider from 80 to 255 and some other changes.
Therefore 100msec is a theoretical option only, as written in my comment.
You are right, your test is a valid test. Anyway, a logic analyzer would have been a help for you, to give some more information about the problem. You would have found a pulse, much shorter than expected. So I had to waste my time for that.
Some options are comin to my mind:

  • live with the port the way it is, setTimeout is an valid option
  • use ESP8266, since this meets your requirement better
  • rewrite the C-source by yourself, and create a pull request so that others benefit from your work. Same way, you benefit from other peoples work.
  • find somebody who is doing this for you, may be you have to pay for that (to avoid misunderstandings. I'm 100% NOT interested )

To compare 2 ports of Espruino, for ESP8266 and ESP32, is not helpful. Both are not official supported by Gordon. Both are the result of some "crazy" peoples work. These guys spent a lot of time porting Gordons beautiful work to other hardware.
BTW, did you already support Gordons great work ?

@gfwilliams
Copy link
Member

I think for anyone who cares enough there's probably enough info here to fix this.

Rather than changing the divider in https://github.com/espruino/Espruino/blob/master/targets/esp32/jshardwarePulse.c#L97 someone could probably just add while (duration>32767) { setPulseHigh(32767); duration-= 32767; } to setPulseHigh (and same for low)

@gfwilliams gfwilliams reopened this Jul 19, 2017
@jumjum123
Copy link
Contributor

jumjum123 commented Jul 19, 2017 via email

@gfwilliams
Copy link
Member

Well, on other (non-ESP8266) platforms, 'util timer' is implemented - it uses one hardware timer to schedule an interrupt to be called at a certain time. The ESP8266 implementation was a massive hack because nobody could figure out how to get a timer & interrupt working.

If you could implement that properly on ESP32 it'd be by far the best solution - then you get Waveform, soft PWM, proper digitalPulse, and any other stuff that comes along later on as well... For instance micro:bit uses it to scan out a matrix LED display.

Once util timer is done, just copy/paste the STM/NRF52 code: https://github.com/espruino/Espruino/blob/master/targets/nrf5x/jshardware.c#L718

Then you can schedule a digitalPulse as long as you want :)

@jumjum123
Copy link
Contributor

Sometimes I believe, the more I learn about Espruino, the less I know at the end.
With other words, util timer was already implemented. To make the story more curious, it was implemeted by me ;-)
As you mentioned, I only had to copy/paste the code from targets/nrf5x/jshardware.c and it worked without any changes 👍
I'm now working on changes to get actual esp-idf working with partition table from @wilberforce and get this documented. This should also be implemented for EspruinoBuildTools. Once this is done changes will be pulled to ESP32 branch.
One more problem is size of bin file, which was about 877KB and is 924KB now.
Is it new esp-idf or use of util timer, which caused this, have to figure out.

@gfwilliams
Copy link
Member

gfwilliams commented Jul 24, 2017

Wow, great news! Using the util timer will be a huge amount better, and will mean it works much more like the other Espruino boards!

I guess we could close this then? Could you come up with a PR for it?

@jumjum123
Copy link
Contributor

Once, everything is uploaded, we can close.
I would prefer to combine with other changes, if this is ok for you.

@gfwilliams
Copy link
Member

Yes, no problem.

@wilberforce
Copy link
Member

@jumjum123 are there other timers that could use the util timers too?

@wilberforce
Copy link
Member

@jumjum123
Does export RTOS=1 have to be added to get the UtilTimerTask task;

https://github.com/espruino/Espruino/blob/master/targets/nrf5x/jshardware.c#L734

?

@jumjum123
Copy link
Contributor

@wilberforce
RTOS=1 is used only to add javascript wrapper for timer, task etc.
Source for internal RTOS functions is always included.
Even without setting RTOS, we use RTOS functions for tasks, timer, notification, UART

@wilberforce
Copy link
Member

@jumjum123

As you mentioned, I only had to copy/paste the code from targets/nrf5x/jshardware.c and it worked without any changes 👍

I tried the code you sent me with the esp-idf 2.0 with a led and digitalPulse(D21,1, 100) with RTOS=1 and it Is working.

I'm move the timer so it's not depending on RTOS and clean up the make file and the files we don't need any more - probably on the weekend..

@wilberforce
Copy link
Member

@jumjum123

#ifdef RTOS
queues_init();
tasks_init();
task_init(espruinoTask,"EspruinoTask",20000,5,0);
task_init(uartTask,"ConsoleTask",2000,20,0);
task_init(timerTask,"TimerTask",2048,19,0);
#else
xTaskCreatePinnedToCore(&espruinoTask, "espruinoTask", 20000, NULL, 5, NULL, 0);
xTaskCreatePinnedToCore(&uartTask,"uartTask",2000,NULL,20,NULL,0);
xTaskCreatePinnedToCore(&timerTask,"timerTask",2048,NULL,19,NULL,0);
#endif

As we need the timers, wondering if we need the #ifdef RTOS or just include this code now?

@gfwilliams
Copy link
Member

Just checking - does this work even if the interpreter is busy?

digitalPulse(LED1,1,100);
var i=100000;
while (i--);

If it's using the RTOS then I guess it's possible it might not run in an IRQ?

@wilberforce
Copy link
Member

yes - this works - after a 1/2 sec the led goes off while stuck in the loop - so it it working off IRQ

digitalPulse(D21,1,500);
var i=100000;
while (i--);

@gfwilliams
Copy link
Member

gfwilliams commented Jul 27, 2017

Great! That's really good news - so presumably analogWrite(LED,0.5,{soft:true}) works as well.

(Not that you need to try it, but proper IRQ-based timers are one of the things that the ESP8266 port has always struggled with)

@wilberforce
Copy link
Member

@gfwilliams
Looks like @jumjum123 has used the esp-idf library - so I don't think this is implmented using the utiltimers

>analogWrite(D21,0.5,{soft:true})
E (34842) ledc: requested frequency and bit depth can not be achieved, try reducing freq_hz or bit_num. div_param=0
=undefined
>analogWrite(D21,0.1,{soft:true})
E (77425) ledc: requested frequency and bit depth can not be achieved, try reducing freq_hz or bit_num. div_param=0
=undefined
>analogWrite(D21,0.05,{soft:true})
E (87018) ledc: requested frequency and bit depth can not be achieved, try reducing freq_hz or bit_num. div_param=0
=undefined

The LED does change intensity, however if we can make this use the espruino libs it would be better

@wilberforce
Copy link
Member

(Not that you need to try it, but proper IRQ-based timers are one of the things that the ESP8266 port has always struggled with)

Is there any reason @jumjum123 's ESP32 code for this can't be ported to the ESP8266 so that Utiltimer works for that?

(probably a different issue required here)

@gfwilliams
Copy link
Member

ledc: requested frequency and bit depth can not be achieved

Ahh - to be honest I think that's as easy as copy/pasting the initial jshAnalogOutput code into ESP32 as well :)

Is there any reason @jumjum123 's ESP32 code for this can't be ported to the ESP8266

If the APIs are exposed then that'd be great! I got the impression that ESP8266 didn't provide proper hardware timers, but I could be wrong... I did see some code for software PWM that seemed to use them, but nobody seemed too bothered about porting it - there may even be an open bug about it.

@MaBecker
Copy link
Contributor

For ESP8266 there is a replacement for the SDK PWM code, maybe this is helpful too.

https://github.com/StefanBruens/ESP8266_new_pwm

@gfwilliams
Copy link
Member

gfwilliams commented Jul 28, 2017

Yeah, so what you'd want to do for ESP8266 is look in https://github.com/StefanBruens/ESP8266_new_pwm/blob/master/pwm.c, pull out all the stuff to do with pwm_intr_handler and the timer registers, and shoehorn that same code into the util_timer stubs in jshardware.c... Then ESP8266 PWM/waveform/etc would be loads better

@MaBecker
Copy link
Contributor

Sorry not me, way above my head ..... maybe not for @wilberforce or @jumjum123 :)

@wilberforce
Copy link
Member

@gfwilliams

Is there any reason @jumjum123 's ESP32 code for this can't be ported to the ESP8266 so that Utiltimer works for that?

Ahh - to be honest I think that's as easy as copy/pasting the initial jshAnalogOutput code into ESP32 as well :)

Do you mean jshPinAnalogOutput ?

This looks like it uses specific hardware timers depending on the implementation?

@gfwilliams
Copy link
Member

Yes - it does - but if it's called with the 'software' flags then it defers to jstimer.c: https://github.com/espruino/Espruino/blob/master/targets/nrf5x/jshardware.c#L643

@jumjum123
Copy link
Contributor

Tried to use jstPinPWM and ran into tons of questions.
Would be great, if you find some time to help me getting a better understanding.

  1. soft and forcesoft are available. Is there a flag for pins to say "supports soft" ?
  2. cannot find jshPinSoftPWM (in line 641), so tried without. Checked all source files, or did I miss something ?
  3. tested with pin D25 first, but this pin is designed as DAC. Right now both DAC pins on ESP32 are hardcoded to use DAC only, is there a better way ?
  4. next tested with LED D5. LED is off, until entering D5.set(), then I get some flickering. Did not test with logic analyzer yet. After entering a character in webide, LED stays off until entering D5.set() again.
  5. tested with D22 and D5 next and got totally strange behaviour with on/of/flickering

May be implementation of util timer is strange, therefore next couple of questions came up to me.
a. ther is only one timer, is it possible to use more than one pin for soft PWM ?
b. how would we stop using util timer for analogWrite ?
c. is there any other function in espruino core using util timer, may be around reading input ? ?
d. will D5.set() stop analogWrite ?

@gfwilliams
Copy link
Member

Hi,

soft and forcesoft are available. Is there a flag for pins to say "supports soft" ?

Hopefully every pin should support soft PWM (since there's no hardware involved at all)

cannot find jshPinSoftPWM (in line 641), so tried without. Checked all source files, or did I miss something ?

BITFIELD_DECL(jshPinSoftPWM, JSH_PIN_COUNT); defines it (in each jshardware.c file) - it's just one bit per pin to say if PWM is enabled - so the timers for it can be cancelled if digitalWrite/etc is used.

tested with pin D25 first, but this pin is designed as DAC. Right now both DAC pins on ESP32 are hardcoded to use DAC only, is there a better way ?

Usually jshPinSetState would set the pin up to the correct state for the peripheral that was to be used. On STM32s we use DAC for analog output by default, but if a frequency is specified it uses PWM instead.

next tested with LED D5. LED is off, until entering D5.set(), then I get some flickering. Did not test with logic analyzer yet. After entering a character in webide, LED stays off until entering D5.set() again.

tested with D22 and D5 next and got totally strange behaviour with on/of/flickering

Sounds like the IRQs for the utility timer might not be doing what you're expecting... Getting it working with a single soft PWM is good though, because you know that it should be firing at specific time intervals.

I guess first steps might be to just try and write the code to set up an IRQ to flash an LED at maybe 1000Hz, in pure C - then when that's working, head towards getting .t working with util timer,

May be implementation of util timer is strange, therefore next couple of questions came up to me.
a. ther is only one timer, is it possible to use more than one pin for soft PWM ?

Yes, the jstimer.c file handles all the scheduling - it's why I do it that way.

b. how would we stop using util timer for analogWrite ?

jshPinState usually kills it, using the bit field:

if (BITFIELD_GET(jshPinSoftPWM, pin)) {

c. is there any other function in espruino core using util timer, may be around reading input ? ?

Yes :) Usually digitalPulse should use it, but also Waveform uses it for input and output, and it's also used to wake the CPU up after a certain time period if it was asleep on most platforms.

d. will D5.set() stop analogWrite ?

It should do, if you've implemented the check fro b in jshPinState :)

@jumjum123
Copy link
Contributor

Wow, a lot of answers only 20 minutes later 👍
I've to work through them step by step now.

@wilberforce
Copy link
Member

@jumjum - looks like you have a lot of homework :-)

@jumjum123
Copy link
Contributor

@wilberforce Yes, a lot of homework, and a lot of (new)open questions ;-)
@gordon I get a negative value for period in calling jshUtilTimerStart
The value is somewhere between #ffffffffffffff6c and #ffffffffffffff70, independent from (analog)value and freq
I expected a positive value. Any idea or explanation ?

@gfwilliams
Copy link
Member

That's odd - how are you calling it? I just tried on nRF52 and I get:

>analogWrite(LED1,0.5,{freq:10,forceSoft:true})  
jshUtilTimerStart 0x0000000000000000
jshUtilTimerStart 0x0000000000000664

@jumjum123
Copy link
Contributor

Hmm, the more I dig into jstimer.c, the more I believe that period should always be negative for first call of jshUtilTimerStart.
In jstPinPWM, taskon.time is set to time, which is jshGetSystemTime()
Some commands later in utilTimerInsertTask we call jshUtilTimerstart with this time minus jshGetSystemTime()
So at the end, we calculate "earlier call to jshGetSystemTime()" minus "later call to jshGetSystemTime()". There should always be a negative difference of some microsecs
Is this correct, or is there a misunderstanding somewhere between my ears ?

@gfwilliams
Copy link
Member

Yes, you're right - it was only working on nRF52 because it uses a 32k oscillator so can go all the way through without the system time changing!

Sorry about that. It looks like if time is <=0 you just have to make sure the timer calls the IRQ as soon as possible (it's probably safer doing that than trying to call it outside the IRQ)

@jumjum123
Copy link
Contributor

I will add a if(period < 10) period = 10
BTW, could'nt the same happen even with 32k, at least in worst case (if command starts some microseconds before next tick) ?

@wilberforce
Copy link
Member

@jumjum123

Did you changes here:
#1214

Address this issue - it appears to be working for me....

If so - we can close this.

@jumjum123
Copy link
Contributor

jumjum123 commented Aug 15, 2017 via email

@chevdor
Copy link
Author

chevdor commented Aug 16, 2017

Happy to confirm that I tested a new build and I can now see it working properly (I am also now using proper tools to measure @jumjum123 :) no LED was harmed in this test).
Thanks for the hard work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ESP32 This is only a problem on ESP32-based devices
Projects
None yet
Development

No branches or pull requests

5 participants