search

arduino-libraries / Servo

Servo Library for Arduino
http://arduino.cc/
GNU Lesser General Public License v2.1
257 stars 263 forks source link

Unor4 timer period updates #116

Closed KurtE closed 10 months ago

KurtE commented 1 year ago

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.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

github-actions[bot] commented 1 year ago

Memory usage change @ 604d84f43f14960fdbc519546982a95f541d7f83

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:mega 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nano33ble 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:megaavr:nona4809 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/Knob`
flash|%|`examples/Knob`
RAM for global variables|%|`examples/Sweep`
flash|%|`examples/Sweep`
RAM for global variables|% -|-|-|-|-|-|-|-|- `arduino:avr:leonardo`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:mega`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:nano`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nano33ble`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nanorp2040connect`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_portenta:envie_m7:target_core=cm4`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:mbed_portenta:envie_m7`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:megaavr:nona4809`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:sam:arduino_due_x_dbg`|0|0.0|N/A|N/A|0|0.0|N/A|N/A `arduino:samd:mkrzero`|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Knob
flash,%,examples/Knob
RAM for global variables,%,examples/Sweep
flash,%,examples/Sweep
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:mega,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nano33ble,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:megaavr:nona4809,0,0.0,0,0.0,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0 ```
iabdalkader commented 1 year ago

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 updateClockPeriod, why can't you use set_period_us ? If you need a different function you can also send it to the core.

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 =, and between cast and type).

github-actions[bot] commented 1 year ago

Memory usage change @ 02c5ee9978f354492f27ab1ff318f7c459754e8e

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:mega 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nano33ble 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:megaavr:nona4809 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/Knob`
flash|%|`examples/Knob`
RAM for global variables|%|`examples/Sweep`
flash|%|`examples/Sweep`
RAM for global variables|% -|-|-|-|-|-|-|-|- `arduino:avr:leonardo`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:mega`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:nano`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nano33ble`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nanorp2040connect`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_portenta:envie_m7:target_core=cm4`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:mbed_portenta:envie_m7`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:megaavr:nona4809`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:sam:arduino_due_x_dbg`|0|0.0|N/A|N/A|0|0.0|N/A|N/A `arduino:samd:mkrzero`|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Knob
flash,%,examples/Knob
RAM for global variables,%,examples/Sweep
flash,%,examples/Sweep
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:mega,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nano33ble,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:megaavr:nona4809,0,0.0,0,0.0,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0 ```
KurtE commented 1 year ago

@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.

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 updateClockPeriod, why can't you use set_period_us ? >If you need a different function, you can also send it to the core.

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 core method:

        uint8_t type = 0;
        int8_t channel = FspTimer::get_available_timer(type);

returned the information to you about which type of timer that was reserved as well as which channel. As to allow your client code to detect such things like, is it GPT or not. And likewise, if it is a 32 bit or 16 bit GPT timer.

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.

github-actions[bot] commented 1 year ago

Memory usage change @ 04b9322aff6d27d5cbb9f53cd7307206a968a0ca

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:mega 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nano33ble 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:megaavr:nona4809 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/Knob`
flash|%|`examples/Knob`
RAM for global variables|%|`examples/Sweep`
flash|%|`examples/Sweep`
RAM for global variables|% -|-|-|-|-|-|-|-|- `arduino:avr:leonardo`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:mega`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:nano`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nano33ble`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nanorp2040connect`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_portenta:envie_m7:target_core=cm4`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:mbed_portenta:envie_m7`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:megaavr:nona4809`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:sam:arduino_due_x_dbg`|0|0.0|N/A|N/A|0|0.0|N/A|N/A `arduino:samd:mkrzero`|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Knob
flash,%,examples/Knob
RAM for global variables,%,examples/Sweep
flash,%,examples/Sweep
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:mega,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nano33ble,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:megaavr:nona4809,0,0.0,0,0.0,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0 ```
iabdalkader commented 1 year ago

@KurtE Thanks, will review and test this again after the core PR is merged.

github-actions[bot] commented 1 year ago

Memory usage change @ fc280bbc69246d66cb08b7e38c756ef804ce0ede

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:mega 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nano33ble 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:megaavr:nona4809 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/Knob`
flash|%|`examples/Knob`
RAM for global variables|%|`examples/Sweep`
flash|%|`examples/Sweep`
RAM for global variables|% -|-|-|-|-|-|-|-|- `arduino:avr:leonardo`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:mega`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:nano`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nano33ble`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nanorp2040connect`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_portenta:envie_m7:target_core=cm4`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:mbed_portenta:envie_m7`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:megaavr:nona4809`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:sam:arduino_due_x_dbg`|0|0.0|N/A|N/A|0|0.0|N/A|N/A `arduino:samd:mkrzero`|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Knob
flash,%,examples/Knob
RAM for global variables,%,examples/Sweep
flash,%,examples/Sweep
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:mega,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nano33ble,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:megaavr:nona4809,0,0.0,0,0.0,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0 ```
iabdalkader commented 11 months ago

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 ?

image

KurtE commented 11 months ago

By design to stagger the starts as to try to minimize the startup current... At least that is what most of the servo setups I have seen have mentioned.

iabdalkader commented 11 months ago

By design to stagger the starts as to try to minimize the startup current... At least that is what most of the servo setups I have seen have mentioned.

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.

github-actions[bot] commented 11 months ago

Memory usage change @ 27dd79bcdf31f41c7d83c85b6d8d6620cf8607ef

Board flash % RAM for global variables %
arduino:avr:leonardo 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:mega 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:avr:nano 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nano33ble 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_portenta:envie_m7:target_core=cm4 N/A N/A N/A N/A
arduino:mbed_portenta:envie_m7 N/A N/A N/A N/A
arduino:megaavr:nona4809 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:sam:arduino_due_x_dbg 0 - 0 0.0 - 0.0 N/A N/A
arduino:samd:mkrzero 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table Board|`examples/Knob`
flash|%|`examples/Knob`
RAM for global variables|%|`examples/Sweep`
flash|%|`examples/Sweep`
RAM for global variables|% -|-|-|-|-|-|-|-|- `arduino:avr:leonardo`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:mega`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:avr:nano`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nano33ble`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_nano:nanorp2040connect`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:mbed_portenta:envie_m7:target_core=cm4`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:mbed_portenta:envie_m7`|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A `arduino:megaavr:nona4809`|0|0.0|0|0.0|0|0.0|0|0.0 `arduino:sam:arduino_due_x_dbg`|0|0.0|N/A|N/A|0|0.0|N/A|N/A `arduino:samd:mkrzero`|0|0.0|0|0.0|0|0.0|0|0.0
Click for full report CSV ``` Board,examples/Knob
flash,%,examples/Knob
RAM for global variables,%,examples/Sweep
flash,%,examples/Sweep
RAM for global variables,% arduino:avr:leonardo,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:mega,0,0.0,0,0.0,0,0.0,0,0.0 arduino:avr:nano,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nano33ble,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0 arduino:mbed_portenta:envie_m7:target_core=cm4,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A arduino:megaavr:nona4809,0,0.0,0,0.0,0,0.0,0,0.0 arduino:sam:arduino_due_x_dbg,0,0.0,N/A,N/A,0,0.0,N/A,N/A arduino:samd:mkrzero,0,0.0,0,0.0,0,0.0,0,0.0 ```