Adafruit Motor Shield V2 DC Motor support

[dtex]

I'd like an opinion.

These shields can be stacked and each has its own dedicated PWM driver. When we create our first DC motor on a shield, we also need to initialize the PWM driver. The second DC motor on that shield will use the same PWM driver instance so we don't want to re-initialize it. If a second shield is stacked on, the motors attached to that shield will use a different PWM driver instance. In other words, motors will be grouped by PWM drivers (or shields).

These I2C shields each need a unique I2C address. Each time a motor is initialized the I2C address needs to be passed (so we know which shield it's attached to). I could use the existing priv map in motor.js to store a reference to the PWM driver using the board's I2C address as the key, but there's a catch. If a user also attaches a stepper motor on the same shield, the PWM driver instance will not be available there.

I don't want to put us in a position where a user can't initialize a DC Motor and Stepper motor on the same shield so I need to store that PWM driver reference in a place where it is available to both classes. That would be the board object. If board had a map where I could store PWM drivers then they would be available to both Motor and Stepper.

That seems drastic so I'm putting it out here for discussion. Maybe there is a better way?

A Shield() class perhaps?

Relates to #380

[rwaldron]

If a second shield is stacked on

Do you have two of these?

I'm going to pull out some important points from your write up and list questions and comments underneath.

These I2C shields each need a unique I2C address. Each time a motor is initialized the I2C address needs to be passed (so we know which shield it's attached to).

• We should manage the addresses from a "pool"
• How does the user initial a single motor on a given shield? Ideally it's the same as now, with some additional or alternate initialization. I see that Adafruit uses a "port" value, so something like:

Adafruit_MotorShield AFMS = Adafruit_MotorShield();
Adafruit_DCMotor *myMotor = AFMS.getMotor(1);

Translates to:

var motor = new Motor({
   device: "AFMSv2",  // this is bikesheddable
   port: 1
});

From their tutorial page:

with getMotor(port#). Port# is which port it is connected to. If you're using M1 its 1, M2 use 2, M3 use 3 and M4 use 4

That would be the board object. If board had a map where I could store PWM drivers then they would be available to both Motor and Stepper.

That's what I was thinking just as I got to this part :)

That seems drastic

I think it will be ok. Create something like this:

Board.Drivers = {
  PWM: <Map>
... other things we might need later?
};

A Shield() class perhaps?

I see that's how they've designed the "official" library.

• It's confusing that sometimes you'd use the regular Motor and Stepper class and sometimes you'd use a Shield class. Unless the Shield class produces instances that were meant to be passed into our Motor and Stepper classes?
• Would we have to provide a Shield device profile for all shields, even if they don't really need it?
• Would we have have to maintain two sets of almost-same API?

I think we should figure out how to do this without packaging as a new class that duplicates a few others. If that idea sucks and is too failure prone, we'll back up and try something different.

[dtex]

Yes, I have two of these.

We should manage the addresses from a "pool"

Even though this is is the only I2C motor shield I'm aware of, I'm building it with the expectation that there may be others out there. This one has 32 possible addresses, but other shields could have very different ranges. If we create a pool now then we limit the code to this shield.

Using the manufacturers motor number is certainly logical, but again I'm assuming there are/will be other I2C boards out there. Different manufacturers use different conventions (Motors A and B instead of 1 and 2 for example). Even if the conventions were the same they would likely map to different pins.

For the API surface I have the interface opts property already in place for I2C and latch (planned ahead). On the back end, this shield is still a 3-pin h-bridge just like our existing CDIR motor so I had envisioned it working like this:

motor = new five.Motor({
    interface: "I2C",
    address: 0x60,
    pins: {
      pwm: 8,
      dir: 9,
      cdir: 10
    }
  });

Then if we wanted to create a specific Adafruit V2 wrapper we could. This is also true for all the other shields we already support.

... other things we might need later?

I like that it could be useful for other things.

I think we should figure out how to do this without packaging as a new class that duplicates a few others.

Excellent point. If you're comfortable with adding a map on the board, then I think the shield class is unnecessary. Our existing API will work with the addition of an address property on the motor opts.

[rwaldron]

Even though this is is the only I2C motor shield I'm aware of, I'm building it with the expectation that there may be others out there. This one has 32 possible addresses, but other shields could have very different ranges. If we create a pool now then we limit the code to this shield.

Good point. While thinking about this just now, I found this: http://www.spinics.net/lists/linux-i2c/msg06519.html which is unrelated, but interesting.

Using the manufacturers motor number is certainly logical, but again I'm assuming there are/will be other I2C boards out there. Different manufacturers use different conventions (Motors A and B instead of 1 and 2 for example). Even if the conventions were the same they would likely map to different pins.

True, I think I was too focused on device-specific profiles—which would not be a problem with different types of port naming, it would just be a bad design resulting in some Motor instances initializing with port: 1 and others with port: "A"—the "syntax" (in the grammatical sense, not PL sense) is different, the semantics are the same.

motor = new five.Motor({
    interface: "I2C",
    address: 0x60,
    pins: {
      pwm: 8,
      dir: 9,
      cdir: 10
    }
  });

+1, I actually didn't realize that the pins could be addressed this way with the I2C device. I suppose if I spent less time making fun of the Adafruit library code and more time reading it... ;)

I like that it could be useful for other things.

Make it so.

Our existing API will work with the addition of an address property on the motor opts.

Excellent.

I think you're off to a great start here. I look forward to seeing what you come up with

[dtex]

Looking back to @soundanalogous comment on a firmata issue, we should be able to set the min and max pulse width. I'm thinking about this because I can adjust the min and max width on each of the Adafruit PWM drivers independently of the values in Firmata and the syntax for that should mirror anything that might get added to board. Have you given that thought already?

Also, would being able to do so address #412?

[rwaldron]

Firmata.js would need to be upgraded to support servo config.

[rwaldron]

Adding this would also mean that all the io-plugins would need to be upgraded (capabilities permitting) as well. Let's punt on servo config for now. We can do it in another issue and better coordinate between all the projects.

[dtex]

Good progress (Spinning a motor at will via I2C) just need to bring that into the API, but I want to make sure I understand something correctly.

There is no real difference in passing a number in hex to sendI2CWriteRequest vs. decimal. The former is commonly done because that is how the memory addresses are described, not because there is a functional difference.

Is that correct?

[dtex]

Hmmm, another concern. The Adafruit shield uses an NXP PCA9685 controller for PWM and much of what I'm doing is specifically for that controller. Other I2C controllers may have different requirements.

It would be more future proof to use

motor = new five.Motor({
  interface: "PCA9685",
  address: 0x60,
  pins: {
    pwm: 8,
    dir: 9,
    cdir: 10
  }
});

[rwaldron]

There is no real difference in passing a number in hex to sendI2CWriteRequest vs. decimal. The former is commonly done because that is how the memory addresses are described, not because there is a functional difference.

Is that correct?

Correct.

[rwaldron]

motor = new five.Motor({
  interface: "PCA9685",
  address: 0x60,
  pins: {
    pwm: 8,
    dir: 9,
    cdir: 10
  }
});

+1, but if we're going with a model number, let's change the key from interface to device to match other classes that also use the model number. In the future we may just switch to calling it model and deprecate device.

What do you think about aliases? eg:

motor = new five.Motor({
  interface: "AFMSv2",
  address: 0x60,
  pins: {
    pwm: 8,
    dir: 9,
    cdir: 10
  }
});

...would have the same meaning? I haven't really thought this through, so I'll leave it to you. "AFMSv2" doesn't actually have to be the alias, I'm just using that for illustration. My thinking is: yes let's use the appropriate model number, but can we make it easier to use by accepting intuitive aliases? This could literally be something like:

motor = new five.Motor({
  device: "aDaFruit Motor SHield v2", // ;)
  address: 0x60,
  pins: {
    pwm: 8,
    dir: 9,
    cdir: 10
  }
});

Which is just case normalized and mapped to PCA9685?

[dtex]

let's change the key from interface to device to match other classes

We already have device in this class. It is implicit based on number of pins passed so it's rarely seen, but it can also be passed explicitly. Device is dependent on the number of pins used to control a motor (Non-Directional = 1, Directional = 2, CDIR = 3). Interface augments/decorates device and controls how we actually write to those pins.

For example, the Adafruit controlled motors' device = "CDIR" and interface = "PCA9685". It uses properties and methods of both.

I'm cool with the idea of aliases and considered that, but I was also thinking that we should keep shields at a higher level and do something like this for shield specific definitions:

AFMS = new five.Shield({
  device: "AdafruitMotorShieldV2",
  address: 0x60
});
motor = new AFMS.motor(1); // "1" maps to Adafruit's motor ID that's printed on the board

AFMS.motor() would know the right pins, and interface type based on the shield's device property and would return a regular Motor() object.

Shield "profiles" could be individual json files that are read/loaded by shield.js as needed.

[rwaldron]

We already have device in this class. It is implicit based on number of pins passed so it's rarely seen, but it can also be passed explicitly. Device is dependent on the number of pins used to control a motor (Non-Directional = 1, Directional = 2, CDIR = 3). Interface augments/decorates device and controls how we actually write to those pins.

Yeah, I knew that, I was just testing you to see if you knew that ;)

new five.Shield

I didn't I didn't realize that this was going forward and I'm still not thrilled about it. In addition to the concerns I mentioned above, I feel like we'll be obligated to make Shield profiles for every shield (not just motor shields) and that gives me feelings of dread. I like that Johnny-Five can be made smart enough to know how to seamlessly interoperate with disparate hardware designs with minimal initialization requirements.

If the Shield instance api is really just a collection of instances initialized from our existing classes, then enough of my concerns are addressed.

[dtex]

I didn't I didn't realize that this was going forward and I'm still not thrilled about it.

There are lots of things in my head that will never see the light of day, and I haven't moved forward on it at all. I think I share your vision that motor.js is completely manufacturer agnostic and can be made to work with most any shield. That's part of the reason I was trying to not address the Adafruit by name in motor.js.

I do envision shield objects as being a collection of existing J5 classes, but they don't even have to be part of Johnny-Five. They can be their own packages.

After support for I2C motors lands I'll throw together a shield profile as a separate issue and we can discuss how that might work.

[rwaldron]

That's part of the reason I was trying to not address the Adafruit by name in motor.js.

Then I appreciate that you push back whenever I say silly things :)

I do envision shield objects as being a collection of existing J5 classes,

Cool

After support for I2C motors lands I'll throw together a shield profile as a separate issue and we can discuss how that might work.

I like this plan.

[dtex]

I2C motors are working, but only some of the time (like only 10%). I believe it's a problem with how quickly commands are being sent. I may have to use the sleep() function from your LCD class in I2C Motors. I know you aren't fond of that approach so I thought I'd see if you had uncovered an alternative.

[rwaldron]

How long do the delays need to be?

[dtex]

500 μs

[rwaldron]

Have you tried wrapping call in a nextTick?

[dtex]

I worried that it wouldn't guarantee 500 μs. I know it would usually give you that and more, but it seemed possible that sometimes it might not in which case the board initialization would fail.

I'll give it a shot and watch out to see if that happens.

I'm also going to have to add a ready event to motor. The board has to be initialized before any commands can be sent and that involves a data read which uses a callback. I'll of course do it in the class so that the API doesn't change and just queue the user commands until the board(shield) is ready.

[rwaldron]

I worried that it wouldn't guarantee 500 μs.

I doubt that the sleep abomination would either—it's currently only ms resolution.

I'm also going to have to add a ready event to motor. The board has to be initialized before any commands can be sent and that involves a data read which uses a callback.

A couple things...

• This won't be necessary for other motors, correct?
• This appears to be prevalent in I2C devices and really bums me out
• Part of me is tempted to accept the minimal blocking that would occur as a one-time setup cost.

Edit

Regarding sleep, I'm more concerned about making sure no one uses that garbage in their program logic. The more I think about it, the more I begin to accept that one-time costs are probably ok; require is blocking and people are used to putting those everywhere in their code, eg.

function Class() {
  require("superclass").call(this);
}

util.inherits(Class, require("superclass"));

Totally insane, but maybe also costs 500μs.

So with that, I say... fuck it—you can use process.hrtime() to get μs resolution for a sleep. As much of an anti-pattern as it is: don't use the other sleep, I want to get rid of it; just make a sleep in motor.js

[dtex]

This won't be necessary for other motors, correct?

Correct, and my intent was to never make it required in the API even for I2C motors. I would just queue commands that get sent before the I2C controller is initialized and then on ready process the queue.

This appears to be prevalent in I2C devices and really bums me out

It makes sense though. Anytime you have to read a byte, toggle a bit and update that byte before sending more commands you're going to have to do this. Is it wrong to want sendI2CReadRequestSync? I would expect that just about every I2C project is going to need it in the init code.

Part of me is tempted to accept the minimal blocking that would occur as a one-time setup cost.

It's really no different than board initialization.

I'll make my sleep use process.hrtime(). I've got it working reliably now, except for brake. It doesn't look like the Adafolks bothered to implement braking in their library but it sure looks possible to me.

p.s. I've learned a ton about I2C this week. I'm feeling pretty empowered right now.

[rwaldron]

I'm feeling pretty empowered right now.

This makes it all worth it :)

[dtex]

You know what, I'm making this way harder than it needs to be.

I only need to do an I2C read if the board is already configured to use sub-addresses or all call is enabled. It is quite likely that this will never happen in a J5 project so I shouldn't be overcomplicating things. If someone ever does need those features we can just add them to the opts. We don't need a synchronous read, command queue, ready event or any of that.

process.hrtime() works great for the reset.

Stacked shields are working well too.

[rwaldron]

I fully support this approach.

[rwaldron]

https://github.com/rwaldron/johnny-five/commit/255d17a97493b103a9200f67d14275e7e016fa60