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

Update of Arduino Servo library for SAMD21 and SAMD51 #292

Closed
agrommek opened this issue Apr 13, 2021 · 4 comments
Closed

Update of Arduino Servo library for SAMD21 and SAMD51 #292

agrommek opened this issue Apr 13, 2021 · 4 comments

Comments

@agrommek
Copy link

agrommek commented Apr 13, 2021

Hi everybody,

I added some code to the Servo library for better support of SAMD21 and SAMD51 board. The library now supports 12 servos per timer and up to 24 servos total, for both SAMD21 and SAMD51. Tested on Feather M0 and Feather M4. I would like to create a pull request for that, but I am not sure to which repository.

My patch would resolve an issue at the repository for the Arduino Servo library, opened by @Silvanosky in 2019. There is already a long-pending-but-not-yet-merged pull request by @ladyada which fixes part of the problem. Because the pull request was not merged, Adafruit duplicated the Servo library here at Adafruit's fork of the ArduinoCore-samd repository with the modifications. I didn't realize at first that there was an open issue upstream, so I based my code off of the version of the Servo library in Adafruit's ArduinoCore-samd repository.

Now, what would be the best way to get the changed code into the upstream Ardino Servo library?

  • Would Adafruit care to accept a pull requenst to the local version of the Servo library in the ArduinoCore-samd repository and update the pull request upstream?
  • Or should I directly make a pull request upstream, with Adafruit's pull reqeust being rejected?

Before I forget it, here is the link to my code. I would be grateful for feedback. :-)

@ladyada
Copy link
Member

ladyada commented Apr 13, 2021

our servo library works pretty well for samd51 and samd21, we'd probably not take a refactor to our code unless its fixing a serious bug. a small, concise PR would be best.

feel free to submit a PR to upstream. we have no insight as to whether it would be merged

@ladyada ladyada closed this as completed Apr 13, 2021
@agrommek
Copy link
Author

Hi @ladyada

Thank you for your prompt response. I will try to submit a PR upstream.

However, just for the record: There is a serious bug in in servo library for the SAMD chips, although it is not in your code, but in the original Arduino SAMD servo code. This bug cannot be fixed without some refactoring.

Just try to use 12 servos (as advertised by the library) with an average pulse width of more than 1820 microseconds (i.e. more than about 21.85 ms total pulse duration for all servos taken together) on a Feather M0 or Arduino Zero. The pulse widths will be ok, but the PWM period/frequency for the servos will go from 20 ms/50 Hz to more than 35 ms/less than 29 Hz. This can be clearly observed on an oscilloscope. Many servos will not work with this kind of timing, I think. The behavior is due to the overflow of the TC counter value after about 21.85 ms.

I will open a new issue upstream ASAP (i.e. tomorrow or so).

@ladyada
Copy link
Member

ladyada commented Apr 13, 2021

oh yeah we understand what you're talking about. i wonder why we dont turn all the pulses on at once, then turn them off at different times. maybe theres more jitter?

@agrommek
Copy link
Author

I think I know why "one does it like this". To me, this appears to be a legacy thing from radio-controlled cars/planes/etc. with analog electronics. There, one has to time-multiplex different servos (i.e. put the pulses back-to-back) in order to be able to control up to 6 servos with a single radio channel. Why only 6, and not up to 10? There would be room for up to ten 2-ms-pulses in a 20-ms-period, after all. Because radio regulations say, that you are not allowed to transmit nonstop, otherwise the generated sidebands would influence neighboring radio channels (at least this PDF mentions something along those lines).

These radio-remote-control-arguments are, of course, not applicable to micorcontrollers. But old habits appear to die hard... 😄

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

No branches or pull requests

2 participants