Support M5StickC with MPU6886

[takeru]

IMU sensor module in M5StickC is changed SH200Q to MPU6886. I will create PR.

https://github.com/m5stack/M5StickC/pull/35

[Tiryoh]

Hi @takeru, Is there any update on this issue?

[wilberforce]

I now have an m5 stick C with the new imu. I'm looking to add a driver for this.

[Tiryoh]

@wilberforce Great! I have working on it, but still testing. I'd be happy if anyone like you who knows more about moddable than I do make a PR.

[phoddie]

@takeru: I took a quick look at the code. It seems pretty reasonable. I am unfamiliar with the IMU, so my feedback is about structure, not the IMU details.

One small point, it looks like you copied some code that does this:

throw "some error happened";

That is allowed. However, it is unconventional to throw a string. Typically an instance of Error is thrown:

throw new Error("some error happened");

The reboot method might more consistently be named initialize or, perhaps as in another sensor code, enable. It does not appear to restart the IMU, but reinitialize it.

Finally, integration is important. Some builds of the m5 stick c use the SH200Q and others use MPU6886. Ideally the host should support both without requiring the developer to configure it manually. Here's the current code which works well for development of the MPU6886 driver:

    // state.accelerometerGyro = new SH200Q;
    state.accelerometerGyro = new MPU6886;

Here's a simple solution to support both:

    try {
        state.accelerometerGyro = new SH200Q;
    catch {
        state.accelerometerGyro = new MPU6886;
    }

The downside to this is that developers with the MPU6886 will experience an exception -- and a distracting visit to xsbug -- each time they start-up because of the exception. You could use preferences to remember the IMU on the device:

const imu = Preference.get("config", "imu");
if (!imu) {
    try {
        state.accelerometerGyro = new  SH200Q;
        Preference.set("config", "imu", "SH200Q");
    catch {
        state.accelerometerGyro = new MPU6886;
        Preference.set("config", "imu", "MPU6886");
    }
}
else if ("SH200Q" === imu)
    state.accelerometerGyro = new SH200Q;
else
    state.accelerometerGyro = new MPU6886;

That way, there will be an exception on the first boot for the MPU6886 devices, but not on successive boots. (You will need to add the preference module to the manifest to use this.)

Finally, the SMBHold class (which you borrowed from other sensor drivers) seems like it might just be the correct behavior for SMBus in all cases (or at least most). If so, maybe we should merge that into the SMBus class. @andycarle, I believe that was originally your code. What do you think?

[takeru]

I said I'd make a PR, but I can't do it right away, so if anyone can do it, please do it.

PR作るといったけど、すぐにはできないのでもしできる人がいたらやってしまってください。

@phoddie : You are right. I think so too, it should switch SH200Q or MPU6886 automatically. I think MPU6886 should be default.

[phoddie]

@wilberforce - since you have the hardware, maybe you could finish the work @takeru began so we can get this integrated?

[andycarle]

@phoddie Yes, I believe that the SMBHold behavior is correct in general.

At the very least, it is the behavior specified in the SMBus spec. See Figure 29 (page 41) and Figure 37 (page 43), as examples, which show a repeated start condition between the write and the read, but no stop condition. That is the behavior I used in SMBHold.

That is probably reason enough to make this behavior the default for pins/smbus. I'm going to guess that making that change will break something—there's always some I2C device that demands non-standard behavior—but I think it's worth doing. Notably, it is also already the default behavior of io/i2c/smbus.

I'm happy to make that change to pins/smbus (and propose a mechanism for overriding the new default) if we want to go that way.

[phoddie]

@andycarle - Thanks for looking into that.

Given that changing it would match the SMBus specification I think we should go ahead and make the change (and clean-up our modules that a use SMBHold).

No doubt that it could break something -- the I²C universe is full of special cases. If and when we find a failure, we can look at providing a configuration option. That's easy to do, but I'd rather not add it until it will be used.

[Tiryoh]

The reboot method might more consistently be named initialize or, perhaps as in another sensor code, enable. It does not appear to restart the IMU, but reinitialize it.

It seems like reasonable. I think initialize is more familiar. In this step, settings for the gyro scope range and accelerometer range is required, which is defined in "3.1 GYROSCOPE SPECIFICATIONS" and "3.2 ACCELEROMETER SPECIFICATIONS" in the MPU6886 datasheet.

Finally, integration is important. Some builds of the m5 stick c use the SH200Q and others use MPU6886. Ideally the host should support both without requiring the developer to configure it manually. Here's the current code which works well for development of the MPU6886 driver:

  // state.accelerometerGyro = new SH200Q;
  state.accelerometerGyro = new MPU6886;

Here's a simple solution to support both:

  try {
      state.accelerometerGyro = new SH200Q;
  catch {
      state.accelerometerGyro = new MPU6886;
  }

The downside to this is that developers with the MPU6886 will experience an exception -- and a distracting visit to xsbug -- each time they start-up because of the exception. You could use preferences to remember the IMU on the device:

const imu = Preference.get("config", "imu");
if (!imu) {
  try {
      state.accelerometerGyro = new  SH200Q;
      Preference.set("config", "imu", "SH200Q");
  catch {
      state.accelerometerGyro = new MPU6886;
      Preference.set("config", "imu", "MPU6886");
  }
}
else if ("SH200Q" === imu)
  state.accelerometerGyro = new SH200Q;
else
  state.accelerometerGyro = new MPU6886;

That way, there will be an exception on the first boot for the MPU6886 devices, but not on successive boots. (You will need to add the preference module to the manifest to use this.)

This seems reasonable too. It would be nice to switch the SH200Q or MPU6886 automatically.

In addition, as mentioned above, the scale range for the gyro scope and the accelerometer, which is defined as GYRO_SCALER and ACCEL_SCALER (const value) in sh200q.js, should be a switchable value like this. I'm not sure this is the good way for JavaScript, but I am trying to make it switchable like this.

[phoddie]

I read your code for the the GYRO_SCALER and ACCEL_SCALER. The convention in the Sensor API is to use the configure method to change these values. To do that, first make the properties private fields. This is not strictly necessary, however it prevents clients of the class from changing the internal state. Instead of this:

        // Set default scalers
        this.gyroScale = GYRO_SCALER.GFS_2000DPS;
        this.accelScale = ACCEL_SCALER.AFS_8G;

Do this to declare and initialize the private fields:

class Gyro_Accelerometer extends SMBHold {
        #gyroScale = GYRO_SCALER.GFS_2000DPS;
        #accelScale = ACCEL_SCALER.AFS_8G;
        ...
}

Each place that uses this.gyroScale or this.accelScale must be changed to this.#gyroScale or this.#accelScale.

Next modify the configure method to allow the GYRO_SCALER and ACCEL_SCALER to be set:

    configure(dictionary) {
        for (let property in dictionary) {
            switch (property) {
                case "operation":
                    this.operation = dictionary.operation;
                    break;
                case "GYRO_SCALER":
                    this.#gyroScale = dictionary.GYRO_SCALER;
                    break;
                case "ACCEL_SCALER":
                    this.#accelScale = dictionary.ACCEL_SCALER;
                    break;
            }
        }
    }

Here are some examples of how client code would now set these properties.

imu.configure({GYRO_SCALER: GYRO_SCALER.GFS_250DPS});
imu.configure({ACCEL_SCALER: ACCEL_SCALER. AFS_4G});
imu.configure({GYRO_SCALER: GYRO_SCALER.GFS_250DPS, ACCEL_SCALER: ACCEL_SCALER. AFS_4G});

[andycarle]

I've committed the change to pins/smbus.js discussed above. The change will be available in our next open source push.

[wilberforce]

I've made a start on this and started a branch with @Tiryoh 's changes.

Struggling to push to my own repo at this point - grrr.

The balls example for sh200q.js does not seem to run - so I need to get that working on the old hardware first.

I have thought of another way of a seemless set up - a poor man's plug and play.. the sh200q and MPU6886 have unique ic2 addresses. Therefore - the address could be polled and then depending on the response, set up the appropriate driver.

I was thinking along the lines of a class IME - that was a factory, and perhaps took a dictionery of I2c addresses mapped to a class contructor - and would then return an IME object that behaved the same why.

The RTC devices that are currently in moddable could be set up the same way.

The manifest for the setup of the underlying hardware could have an order list of which device to look for first.

In this case since the current hardware is MPU6866, it would be ['"MPU6866','sh200q']

[Tiryoh]

@wilberforce Thanks for your feedback.

The balls example

Is this https://github.com/Moddable-OpenSource/moddable/tree/public/examples/drivers/m5stickc-imu?

I've faced the problem like this. https://twitter.com/Tiryoh/status/1295775595574333440

I have fixed the problem today, on this commit: https://github.com/Tiryoh/moddable/commit/f6948b285c2f164c91c65d29fc089c53a01a5dce It seems like working. https://twitter.com/Tiryoh/status/1297082663925239809

[phoddie]

@wilberforce wrote:

I was thinking along the lines of a class IME - that was a factory, and perhaps took a dictionery of I2c addresses mapped to a class contructor - and would then return an IME object that behaved the same why.

There's a somewhat obscure JavaScript feature that could work here. You can have a constructor that instantiates a class and returns it. Roughly...

import SH200Q from "..."
import MPU6886 from "..."

class IMU {
    constructor(options) {
        if (thisDeviceHasSH200Q)
             return new SH200Q(options);

        return new MPU6886(options);
   }
}

Still, the IMU soldered to the board never changes and the probe will take more time on each boot than reading the preference. So, I kind of prefer the preference approach.

The probe approach would eliminate the exception when determining IMU (set throw to false in the dictionary passed to the I2C constructor). That's great. It could still be worthwhile to remember the IMU discovered so it can be used immediately on subsequent boots.

[wilberforce]

One issue I did note is that on the new hardware - the following uncommented in m5stick_c\setup-target,js

trace('The Temp:', state.accelerometerGyro.sampleTemp(), '\n');

The temperature seems to be 53c and in my case I expect 20c or so...

[Tiryoh]

The temperature seems to be 53c and in my case I expect 20c or so...

This happened on my M5StickC too... https://twitter.com/tiryoh/status/1296451990466244609

[wilberforce]

Pull request: https://github.com/Moddable-OpenSource/moddable/pull/427

Now silently detects without any throw errors:

state.accelerometerGyro = new MPU6886;

    if (!state.accelerometerGyro.ok()) {
        state.accelerometerGyro = new SH200Q;
    }

[phoddie]

We've integrated the PR. Nice work everyone!

I made a few adjustments to that. It seems to be working well (on a borrowed M5Stick).

As a bonus, the manifest now specifies the baud rate of 1500000, so it works without having to manually set the debugger and upload speeds.

[Tiryoh]

I tried the IMU sample (https://github.com/Moddable-OpenSource/moddable/tree/276e962b1106b798b388663f0f311bc32f917c23/examples/drivers/m5stickc-imu) on this commit: https://github.com/Moddable-OpenSource/moddable/commit/276e962b1106b798b388663f0f311bc32f917c23. It worked properly! Thanks, @wilberforce, @phoddie, and everyone who helped with this.

As a bonus, the manifest now specifies the baud rate of 1500000, so it works without having to manually set the debugger and upload speeds.

Thanks, @phoddie! The following command was able to write to the M5Stick-C successfully.

mcconfig -v -d -m -p esp32/m5stick_c