-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fixing end of word, adding support for microseconds #1
Conversation
Very nice job, thanks. I wonder if we could get rid of the old hundredth second based time word and leave only the new microsecond base one. The same applies to the delay. What do you think? Do you use both time and time-us, as well as delay and delay-us? |
Hi!
:) Thanks. But you are the one who did the great job!
ANS Forth only defines the word MS ( n — ) to delay the execution n milliseconds. It has no word for "getting milliseconds from the system“. There is a nice summary of multiple forth dialects at https://www.rosettacode.org/wiki/System_time#Forth for this case. So, I would vote for a new/additional word MS ( n — ) which overtakes DELAY. MS@ ( — ms ) could overtake TIME. With this naming system, we could use US ( n — ) to delay microseconds and US@ ( — n ) to get microseconds from the system. I would not remove the millisecond variant because „everybody“ knows, that ms are used to calculate/convert dates. Additional the microseconds variant is too short (if one CELL is used) aka doesn’t last long enough.
I will try to get more experience in the next days. In my eyes punyforth is „work and fun in progress“, so I think it is acceptable, that not all words/features are perfect at the first time. I am having no problem if words/features are changing if this change is documented.
|
This sounds like a good naming convention, I like it. The only thing I'm not sure about it is the difference between sdk_os_delay_us and vTaskDelay. The current delay implementation uses the latter. If these functions work significantly differently (e.g. if vsTaskDelay blocks only the current task, while sdk_os_delay_us blocks everything) then ms and us doesn't sound so right, because based on the name I would expect that the only difference between them is the quantity of the waiting period. I'll try to figure out the difference between sdk_os_delay_us and vTaskDelay, or if you already know, please let me know. By the way, vTaskDelay(500000/portTICK_RATE_US) isn't the same as sdk_os_delay_us(500000) ?
Yeah, changing it later is always an option. What worries me when I add new words, is that the dictionary space on the esp is quite small (24kb). It can can be quickly filled up by loading 5-6 modules (although this is not the typical case). I can increase it up to 30kb but doing so, could result stability issues because for example LWIP allocates netcon buffers dynamically, and it needs additional space. Maybe some tree shaking or dead code elimination or minification applied to uber.forth will be the ultimate solution for the limited space. |
|
Yeah, I think the same. Anyways, I convinced myself that it's ok :). I still like the ms, ms@, us, us@ naming.
I agree. Hiding the prompt is probably not too difficult, storing the binary dictionary instead of the source code in flash is trickier. At first I went to that direction but ran into problems I can't recall at the moment. There are rooms for improvements in this regards. I merged the code then renamed the followings:
How about renaming fields in the event structure: .time -> .ms and .time-us -> .us ? |
Cool!
Yes! This seems consequent! |
Done. |
This is the response what the developer of esp-open-rtos gave regarding the sdk_os_delay_us and vTaskDelay.
So yes, sdk_os_delay_us is implemented as a busy loop, and vTaskDelay is not suitable for high precision delays. A further improvement could be made by using vTaskEnterCritical / vTaskExitCritical around sdk_os_delay_us to make it more precise. |
Many thanks for your investigations! This are great news. |
This is my first pull request ever, so any comments are appreciated :)
I ran the 'test' module successfully.