omcaree / node-pololumaestro

Node module for control of Pololu Maestro servo controllers
14 stars 16 forks source link

Library refactor, implemented compact protocol and added test suite #2

Closed achingbrain closed 11 years ago

achingbrain commented 11 years ago

I took what you'd done with the setPWM method and fleshed it out to implement the full compact protocol following the docs & also added unit tests with a good level of coverage.

The existing setPWM method used the code 0x84 which appears to set the target of the given servo, rather than 0x8A which sets the value of the PWM output on boards that support it, so now setTarget uses 0x84 and setPWM uses 0x8A in line with the docs - this will break any existing code so I revved the library version number to 2.0.0.

There are a couple of helper classes, the interesting one being SerialPortWrapper which attempts to guarantee writes and reads to the serial port don't overlap.

Finally I renamed the src directory to lib as that seemed to be recommended for pure js npm libraries.

Happy to discuss any changes, let me know what you think.

omcaree commented 11 years ago

Great work! I only ever needed basic servo control so coding up full functionality always took a back seat. Your helper classes definitely improve things too, I wish I'd thought of that!

I was using the function name setPWM as a bit of a legacy to some in house C code I inherited, but I agree that your naming convention makes more sense (being consistent with the docs and all).

Could you add a quick comment to the Readme about the incompatibility between 1.1.0 and 2.0.0? Just so no one gets confused if their code breaks.

Also, feel free to add your name to the "Updates as of v2.0.0" section. Not everyone checks the source history and I don't want to take the credit for your excellent work!

achingbrain commented 11 years ago

I've updated the readme. Added a David badge too..

achingbrain commented 11 years ago

Actually, perhaps you could set up Travis and Coveralls too? I'd do it but I think you might have to make me a contributor first.

omcaree commented 11 years ago

I've added you as a collaborator on this project, so you should be able to make any changes you like now

omcaree commented 11 years ago

Merged, and updated npm package to 2.0.0