adding timer3 functions as servo.* because we need to control bots

[soldair]

after quite a bit of user research we have decided to include timer3 functions as servo.* in master

[jacobrosenthal]

The reason I originally used timer was to have a more general timer3 available for uses including servo, but if you're going to use it JUST for servo you might want to use pjrc's servo implementation which is more clear. EX. servo.write() from 0-180 degrees

https://www.pjrc.com/teensy/td_libs_Servo.html

On Wed, Jun 25, 2014 at 7:22 AM, Ryan Day notifications@github.com wrote:

after quite a bit over user research we have decided to include timer3

functions as servo.* in master

You can merge this Pull Request by running

git pull https://github.com/Pinoccio/library-pinoccio servo-commands

Or view, comment on, or merge it at:

https://github.com/Pinoccio/library-pinoccio/pull/142 Commit Summary

• adding timer3 functions as servo.* because we need to control bots by default

File Changes

• M src/Shell.cpp https://github.com/Pinoccio/library-pinoccio/pull/142/files#diff-0 (52)
• M src/Shell.h https://github.com/Pinoccio/library-pinoccio/pull/142/files#diff-1 (1)
• A src/peripherals/TimerThree.cpp https://github.com/Pinoccio/library-pinoccio/pull/142/files#diff-2 (28)
• A src/peripherals/TimerThree.h https://github.com/Pinoccio/library-pinoccio/pull/142/files#diff-3 (160)

Patch Links:

• https://github.com/Pinoccio/library-pinoccio/pull/142.patch
• https://github.com/Pinoccio/library-pinoccio/pull/142.diff

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/library-pinoccio/pull/142.

[soldair]

thanks! ill take a look. i just changed it to servo.* so its more clear what we intend but not to limit your use cases for timer3

[cyborg5]

I have some concerns about this particular item. I believe that all of this code is going to be compiled into the system whether you use it or not. That might preclude other uses of timer 3 by other code. Also do you really need this code in order to operate a servo? I have connected servos to just about any PWM pin and use the standard Arduino Servo library without having to directly manipulate any hardware registers or using any interrupt service routines.

Especially troublesome is the definition of a "ISR (…) {…}" routine. I had a similar routine in my IRLib library for infrared receiving, decoding, and sending. The ISR routine has to be globally defined. Users were complaining that if they were not using a portion of my library that required the ISR, it was conflicting with their use of an ISR. I had to put in a #define and an #ifdef… #endif around the routine to allow people to disable it if they wanted to.

I've got no problem if this was a separate library that would encapsulate the timer 3 and servo functions that you have included. And have it customized for Pinoccio as you have done. But I don't think it should be part of the built in infrastructure.

[soldair]

Thanks this is exactly the feedback I needed. On Jun 25, 2014 5:31 PM, "Chris Young" notifications@github.com wrote:

I have some concerns about this particular item. I believe that all of this code is going to be compiled into the system whether you use it or not. That might preclude other uses of timer 3 by other code. Also do you really need this code in order to operate a servo? I have connected servos to just about any PWM pin and use the standard Arduino Servo library without having to directly manipulate any hardware registers or using any interrupt service routines.

Especially troublesome is the definition of a "ISR (…) {…}" routine. I had a similar routine in my IRLib library for infrared receiving, decoding, and sending. The ISR routine has to be globally defined. Users were complaining that if they were not using a portion of my library that required the ISR, it was conflicting with their use of an ISR. I had to put in a #define and an #ifdef… #endif around the routine to allow people to disable it if they wanted to.

I've got no problem if this was a separate library that would encapsulate the timer 3 and servo functions that you have included. And have it customized for Pinoccio as you have done. But I don't think it should be part of the built in infrastructure.

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/library-pinoccio/pull/142#issuecomment-47175766 .

[matthijskooijman]

I've got no problem if this was a separate library that would encapsulate the timer 3 and servo functions that you have included.

There's currently no infrastructure for dynamically loading code like this, and it seems that creating it would be very much non-trivial. It could of course be something that's separately enable-able at compiletime (by (not) including a library from the main sketch), which might be what you mean?

I think an easier solution would be to mark the ISR as a weak function, allowing it to be overridden by a strong (non-weak) function in another part of the code.

[cyborg5]

It could of course be something that's separately enabled at compile time…

Yes this is exactly what I'm talking about. The functionality that it provides is not something that the average user is going to meet. You would only need it for a specific application and including it for everyone is just code bloat. But having it as a separate library that could be compiled in as needed is a better idea. I realize you can't dynamically load it on demand. But you can choose to include or not include the library and a custom sketch.

For example I've got a custom sketch that wraps some of my infrared library into a bitlash/Scout script command called "ir.send(protocol, data, bits)". But unless you have a particular application that needs to send IR data that really doesn't need to be part of the overall Pinoccio firmware. Similarly unless I want to use my Pinoccio to control servos or otherwise manipulate TIMER 3 then I don't think it should be part of the built in infrastructure.

Presumably at some point we would have a motor shield backpack and then I would think you would want to always include such a library anytime you are compiling a sketch for that particular backpack. But as a general purpose feature of Scout script I don't think enough people would use it to make it worthwhile.

Just my opinion.

[matthijskooijman]

As for code bloat - including it in the generic Bootstrap seems ok to me, at least until we reach the limits of our Flash space. People who use the compiled Bootstrap firmware aren't going to use the space for anything else.

For people compiling a custom sketch, it makes sense to allow enabling / disabling code. However, this same goes for other stuff - you'd want to disable lead scout code as well for example. So I think this is a separate issue, that shouldn't block merging this pullrequest (see #148).

As for the naming of these functions - I really don't like servo here, since it implies special servo support which simply isn't true. If we want to make people clear that they can use timer3 for servo's, then we should solve that with documentation - not innacurate naming.

[soldair]

I can change the names back. You make good points. We discussed this a little in Tahoe and decided that most people may not know what timer3 us whereas they do know On Jun 30, 2014 2:54 AM, "Matthijs Kooijman" notifications@github.com wrote:

As for code bloat - including it in the generic Bootstrap seems ok to me, at least until we reach the limits of our Flash space. People who use the compiled Bootstrap firmware aren't going to use the space for anything else.

For people compiling a custom sketch, it makes sense to allow enabling / disabling code. However, this same goes for other stuff - you'd want to disable lead scout code as well for example. So I think this is a separate issue, that shouldn't block merging this pullrequest (see #148 https://github.com/Pinoccio/library-pinoccio/issues/148).

As for the naming of these functions - I really don't like servo here, since it implies special servo support which simply isn't true. If we want to make people clear that they can use timer3 for servo's, then we should solve that with documentation - not innacurate naming.

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/library-pinoccio/pull/142#issuecomment-47513942 .

[cyborg5]

I don't care what it's called I still think it should be a standalone library that you have the option to include or not include rather than having to edit some #define to disable. Actually the same thing goes for lead scout versus field scout. The lead scout specific code should all be in one library that gets included only when compiling sketches for leads Scouts.

As far as the other comment about reaching the limits of flash memory, we need to think of this in terms of the Pinocchio core code being the operating system. Would anyone have bought Windows or OS X if the operating system had the attitude of "we will let the code grow until we reach the limits of memory". At some point you have to leave room for the application programs.

The bottom line is this is not a feature that many people are going to use and so it must be extremely easy to include or not include. The easiest way to do that is to either #include or not.

[soldair]

we have been working with people where this is a very important feature to have out of box otherwise this wouldn't have been opened.

the default hex we flash should include more things than we have in "core" so maybe we work on the bundling process.. more to think about =)

[matthijskooijman]

As far as the other comment about reaching the limits of flash memory, we need to think of this in terms of the Pinocchio core code being the operating system. Would anyone have bought Windows or OS X if the operating system had the attitude of "we will let the code grow until we reach the limits of memory". At some point you have to leave room for the application programs.

If there was no way to actually install extra programs to make use of that extra memory (like currently with Pinoccio, if you use the precompiled bootstrap), then I don't think anyone really cared how much memory the operating system uses. Do you care how much flash space the firmware of your TV, wireless router, etc. takes (provided you can't install apps or more of that stuff)?

The bottom line is this is not a feature that many people are going to use and so it must be extremely easy to include or not include. The easiest way to do that is to either #include or not.

I agree on making it optional and I agree that having a separate library is probably the best approach (at least for timer3). However, this is really a separate issue that I'd like to solve later - get things working first. In any case, let's continue this discussion in #148 about how to approach making things optional.

[quartzjer]

I'm going to close this since the code was merged into modules and will be showing up there again soon :)