Unor4 timer period updates #116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of fixed 100us timer, it now changes the timer period on the fly to get better granularity. And like most other Servo implementations, It also only starts one servo at a time, to help minimize current surge to try to start all of the servos at the same time. I have done some testing with one and two servos with the sweep, where the 2nd servo is 180-pos...
Note FSP->AGT timer code is completely busted Will produce PR to hopefully fix it.
|
|
|
Memory usage change @ 604d84f
Click for full report table
Click for full report CSV |
per1234
added
topic: code
|
Hi @KurtE thank you for sending this, I've tested these changes and they do fix the issue it seems. However, we need to keep the FSP/HW specific code out of the library, so can you please add a function that disables the buffer to the core ? In there we can check if it's GPT or AGT, and for the Note please remove all debugging code and the extra empty lines that were added between functions, and please try to match the existing coding style (space before and after |
Removed the #ifdef print code. Also removed a couple of extra blank lines that were added. Found a couple places where I cut and pasted in some variable definitions that did not have spaces around =
|
Memory usage change @ 02c5ee9
Click for full report table
Click for full report CSV |
|
@iabdalkader I did a first pass and removed the debug code, a couple of extra blank lines that were added and a few defines that for some reason did not have blanks around the = in the initializers.
I Will look into adding new methods to the core as to move the hardware specific code out of this hardware specific section of the servo library. I was trying to avoid the necessity of needing a new core in order to fix the servos. Figured that was why the returned the information to you about which type of timer that was reserved as well as which channel. As to allow your client set_period_us - works like a sledgehammer. It stops the timer, then sets up the data in the abstraction data type, then runs through the initialization code and then starts up the timer again. |
KurtE
added a commit
to KurtE/ArduinoCore-renesas
that referenced
this pull request
Sep 10, 2023
During code review of changes I made to the Servo library: arduino-libraries/Servo#116 @iabdalkader requested that I move all of the platform specific timer changes into the core instead of being in the hardware specific sub-directory of the Servo library. This change adds a method to tell the GPT timer, that when I make a change to the pseriod (set_period(p) ) that I want the change to happen now and not be buffered. And updated the set_period method to check for this and directly set the not buffered register with the new period.
I moved the code that disables the period buffer into a new member function. I then checked for this in the set_period(p) call, which will either allow it to go to FSP code, or directly set the appropriate register with the new period. arduino/ArduinoCore-renesas#131
|
Memory usage change @ 04b9322
Click for full report table
Click for full report CSV |
|
@KurtE Thanks, will review and test this again after the core PR is merged. |
|
Memory usage change @ fc280bb
Click for full report table
Click for full report CSV |
facchinm
pushed a commit
to arduino/ArduinoCore-renesas
that referenced
this pull request
Sep 15, 2023
* Add method to allow the GPT Timer period not to be buffered During code review of changes I made to the Servo library: arduino-libraries/Servo#116 @iabdalkader requested that I move all of the platform specific timer changes into the core instead of being in the hardware specific sub-directory of the Servo library. This change adds a method to tell the GPT timer, that when I make a change to the period (set_period(p) ) that I want the change to happen now and not be buffered. And updated the set_period method to check for this and directly set the not buffered register with the new period. Co-authored-by: Ibrahim Abdelkader <i.abdalkader@gmail.com>
|
Hi @KurtE sorry it took so long to get around to this, we've been very busy with some other issues. I see that your changes to the core are now merged, great! I have some comments on this PR, minor typos, styling etc.. and was about to post a full review, but I noticed a small issue with this, do you see how channels are started sequentially ? I think they should all start at the same time. Not sure if this will be easy to fix, would you like to try ?
|
|
By design to stagger the starts as to try to minimize the startup current... |
iabdalkader
requested changes
Jan 3, 2024
Good point, but I think this is outdated or for an older platform that had hardware issues. It's fine with me either way, unless no one else objects, I think this is good to merge after the review comments are fixed. |
Co-authored-by: Ibrahim Abdelkader <i.abdalkader@gmail.com>
Updates the usages of functions or constants that the names were changed
|
Memory usage change @ 27dd79b
Click for full report table
Click for full report CSV |
iabdalkader
approved these changes
Jan 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should resolve: #113
Instead of fixed 100us timer, it now changes the timer period on the fly
to get better granularity.
And like most other Servo implementations, it also only starts one servo at a time, to help minimize current surge to try to start all of the servos at the same time.
I have done some testing with one and two servos with the sweep, where the 2nd servo is 180-pos...
I also found that the FSP using AGT timer was completely busted. I know what is going on and have it working with some core changes. Will create PR to fix that issue.