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

bts7960: Add new device package #447

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

kostyay
Copy link

@kostyay kostyay commented Aug 4, 2022

Added support for bts 7960 motor driver.
It is based on the C++ code from this library: https://github.com/luisllamasbinaburo/Arduino-BTS7960

@kostyay kostyay changed the title Support bts7960 motor driver bts7960: Support motor driver Aug 4, 2022
@kostyay kostyay changed the title bts7960: Support motor driver bts7960: Add new device package Aug 4, 2022
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Can you also add an example and a smoke test? Each driver has an example in the examples directory and a smoke test in the Makefile.

}

// New returns a new motor driver.
func New(lEn, rEn, lPwm, rPwm machine.Pin, pwm machine.PWM) *Device {
Copy link
Member

Choose a reason for hiding this comment

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

Can you define and use a PWM interface instead of using machine.PWM? It is called differently on other chips.

For example, with the following interface:

type PWM interface {
    Channel(pin machine.Pin) (channel uint8, err error)
    Set(channel uint8, value uint32)
    Top() uint32
}
Suggested change
func New(lEn, rEn, lPwm, rPwm machine.Pin, pwm machine.PWM) *Device {
func New(lEn, rEn, lPwm, rPwm machine.Pin, pwm PWM) *Device {

}

// Left turns motor left.
func (d *Device) Left(speed uint32) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, what about Clockwise / Counterclockwise? No strong opinion from me, but it might be easier to understand. (I don't know this motor though so I don't know which terminology makes sense).

Copy link
Author

Choose a reason for hiding this comment

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

The controller has a L and and R pins so I prefer to keep it this way
image

@kostyay
Copy link
Author

kostyay commented Aug 10, 2022

Fixed ur comments

@deadprogram
Copy link
Member

First of all, thanks for working on this.

SO if you look at both the L293x and the BTS7960, it can be noted that they are both H-bridges and as a result have essentially the same sort of way that you connect to them and use them.

As the Wikipedia page says:

These circuits are often used in robotics and other applications to allow DC motors to run forwards or backwards.

The only difference is that the BTS7960 has a connection to enable separately the forward direction from the reverse direction.

I would suggest basically copy the structure/format of the L293x driver. Of course rename the pins, and then implement the same API in the manner that the BTS7960 requires. I propose that we define Motoring interface:

type Motoring interface {
    Forward()
    ForwardWithSpeed(speed uint32)
    Backward()
    BackwardWithSpeed(speed uint32)
    Stop()
}

In the case of the Forward() and Backward() methods, a device with PWM would use full speed.

In the case of the ForwardWithSpeed() and BackwardWithSpeed() methods, a device without PWM would ignore the params and use full speed.

This will make it possible to use either the BTS7960 or the L293X with a minimal amount of code changes.

Last comment, the L293X driver does not try to configure the PWM for the user:
https://github.com/tinygo-org/drivers/blob/release/l293x/l293x.go#L88-L89

This is part because not every board has the same PWM implementation, even though they expose the same interface. Just seems like it simplifies things for users.

What do you think? If it seems like a good idea, I can go back to the L293x and make sure it is doing the same thing.

@kostyay
Copy link
Author

kostyay commented Aug 21, 2022

Hey
Sounds good, I will do it :)

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

Successfully merging this pull request may close these issues.

3 participants