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

Added sabertooth driver #748

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

elie-g
Copy link

@elie-g elie-g commented Apr 14, 2020

I created a GPIO driver for the DimensionEngineer's Sabertooth motor driver.
I made it for my own usage and I thought you might want it.

See here for more information.

Copy link
Collaborator

@gen2thomas gen2thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elie-g thank you for sharing your code with the gobot project. I have finished my review just now. It would be perfect if you could take a few more minutes to add or change a few things.

}

// SabertoothAnalogDriver represents a sabertooth driver in analog mode
type SabertoothAnalogDriver interface {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you remove the interface and add the comments to the related exported functions.This is, because there is no further implementation of it and introducing of interfaces in golang is a little bit different according to other languages.

After that you could most likely also remove the "// END" comments in the code, because the beginning of the next function is marked by the description comment.

Reading this post will help to understand my remark.

// Adds the following API commands:
// throttle(value float64) - see SabertoothAnalogDriver.Throttle
// differential(value float64) - see SabertoothAnalogDriver.Differential
func NewSabertoothAnalogDriver(w PwmWriter, d SabertoothDIPSwitches, s1, s2, a1, a2 string) SabertoothAnalogDriver {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you add the link to the provided references in your commit message here in the code. This will simplify future development. You can have a look to i2c drivers for some examples.

A link to an reference code in another programming language is also helpful for future error catching, e.g. for i2c drivers there are often drivers already implemented in python or for arduino in C/C++.


// NewSabertoothDIPSwitches creates a new SabertoothDIPSwitches given the
// position (true: ON, false: OFF) of the DIP switches 3, 4, 5 and 6.
func NewSabertoothDIPSwitches(powerSupply, mixed, linear, bidirectional bool) SabertoothDIPSwitches {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example (/gobot/examples/*) would be nice to understand how to use the dip switches together with the driver.

return s.write(s.a2, ratio)
} // END SetMaxPower

func (s *sabertoothAnalogDriver) Throttle(throttle float64) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks quite complicate, so my suggestion is to add some unit test.

}
} // END Throttle

func (s *sabertoothAnalogDriver) Differential(angle float64) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks also quite complicate, so my suggestion is to add some unit test.

@gen2thomas
Copy link
Collaborator

Set back to draft for later reference.

@gen2thomas gen2thomas marked this pull request as draft November 30, 2022 18:36
@gen2thomas gen2thomas added maintainer wanted the implementation was started, but for some reason not finished and removed in review labels Nov 30, 2022
@gen2thomas gen2thomas deleted the branch hybridgroup:dev May 15, 2023 16:25
@gen2thomas gen2thomas closed this May 15, 2023
@gen2thomas
Copy link
Collaborator

accidentally closed - now reopen

@gen2thomas gen2thomas reopened this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer wanted the implementation was started, but for some reason not finished
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants