Magnetic sensor ma730

[owennewo]

Added support for MA730.
Added clock_speed configurability to spi and i2c
Allow i2c sda/scl pins to be reconfigured
Allow spi mode to be changed
Provide helper static constructors that hides things like needing to know registers, resolutions chip_address e.g:

MagneticSensorSPI sensor1 = MagneticSensorSPI::AS5147(PIN_MAG1);
MagneticSensorSPI sensor2 = MagneticSensorSPI::MA730(PIN_MAG1);
MagneticSensorI2C sensor3 = MagneticSensorI2C::AS5600();

@askuric - I'm happy to discuss static constructors as you may have a better way or see other problems with this approach.

[askuric]

Nice job owen!

But I am not sure this static constructor is the best idea. I agree it is very simple to use though.
First are you sure that you can have more than one sensor called by the same static constructor. Will this work:

MagneticSensorSPI sensor1 = MagneticSensorSPI::AS5147(PIN_MAG1);
MagneticSensorSPI sensor2 = MagneticSensorSPI::AS5147(PIN_MAG2);

How I was imagining it was more like this:

MagneticSensorSPI sensor = MagneticSensorSPI(AS5147, CS_PIN);

And to have:

struct SensoConfig_s{
 .... // all that we need
};
SensoConfig_s AS5147 = {...};
SensoConfig_s AS5600 = {...};

I am not sure that it is a better idea any more. The only thing that is maybe interesting is that the user can define their own sensor config in the arduino sketch and then provide it to the constructor. With dedicated constructors it is a bit more complicated. And if we want to support more sensors (AS5047 and AS5048B - the same settings) we need a constructor for each one.

[owennewo]

I've checked the following for you:

MagneticSensorSPI sensor1 = MagneticSensorSPI::AS5147(PIN_MAG1);
MagneticSensorSPI sensor2 = MagneticSensorSPI::AS5147(PIN_MAG2);

It works. I suspect it would behave as you feared if the static function did not use the new keyword.

I also tried the above with different SPI speeds. e.g. sensor1 at 1K clock_speed and sensor2 at 1M clock_speed. This worked but going lower than 1k made the faster sensor stop working. I think most people would stick to a single clock speed but I don't think the spec requires it.
It's also possible to something similar for BLDCMotor e.g.

BLDCMotor motor = BLDCMotor::SimpleFocShieldLayout1(pole_pairs); // defaults to some pin combo defined in docs

So given that the above works then it's really down to which code style we prefer.

[owennewo]

I've tried with an i2C as well:

MagneticSensorSPI sensor1 = MagneticSensorSPI::MA730(PIN_MAG1);
MagneticSensorSPI sensor2 = MagneticSensorSPI::AS5147(15);
MagneticSensorI2C sensor3 = MagneticSensorI2C::AS5600();

void setup() {

  Serial.begin(115200);

  sensor1.init();
  sensor2.init();
  sensor3.init();

}

void loop() {
  Serial.print(sensor1.getAngle());  Serial.print("\t"); 
  Serial.print(sensor2.getAngle()); Serial.print("\t"); 
  Serial.println(sensor3.getAngle());
}

Works too.

[owennewo]

I agree that your config approach looks tidy too - Propbably need a static class prefix on the well known configs e.g: MagneticSensorSPI::MA730.

MagneticSensorSPI sensor1 = MagneticSensorSPI(MagneticSensorSPI::MA730, PIN_MAG1);
MagneticSensorSPI sensor1 = MagneticSensorSPI(MagneticSensorSPI::AS5147, PIN_MAG1);
MagneticSensorI2C sensor2 = MagneticSensorI2C(MagneticSensorI2C::AS5600, PIN_MAG1);

I'd be happy with that too. And as you say - it would allow developers to create their own. I'm leaning slightly to your approach - I like how the config struct defines what can be changed.

[askuric]

I am just not sure that either BLDCMotor or MagneticSensor* should have any of the hardcoded configuration in them directly.

Honestly I would prefer for these classes to as configurable as possible (generic is a better word maybe) and then to have a configuration instance that they receive over the constructor for each new sensor.

As well as I would prefer to create a class for the SImplefocShield that incorporates BLDMotor instance.
Because FOC code should remain generic and BLDC driver agnostic.

But I agree that having these configuration already done would help people spend less time reading the docs.

[owennewo]

How about the sensor has the struct definition in it and then a new SensorLibrary.h has some struct instances?

[owennewo]

@askuric - I've pushed commit with struct based approach.

MagneticSensorSPI sensor1 = MagneticSensorSPI(MA730_SPI, PIN_MAG1);
MagneticSensorSPI sensor2 = MagneticSensorSPI(AS5147_SPI, 15);
MagneticSensorI2C sensor3 = MagneticSensorI2C(AS5600_I2C);

The 3 struct instances listed above are defined in a new Hardware.h file
WDYT?

[askuric]

Hey Owen, great work!
I like it. I am only not sure about the header file name "hardware.h".
But this is easy to change later if we decide to.

[owennewo]

I thought of a few other names such as Catalog.h or HardwareConfigs.h.

I don't mind what it is really. Feel free to make changes.