arduino-libraries / Arduino_PortentaBreakout

GNU Lesser General Public License v2.1
10 stars 5 forks source link

Breakout I2C UART SPI AnalogRead AnalogWrite #9

Closed pennam closed 3 years ago

pennam commented 3 years ago

Sorry guys @facchinm @Rocketct, but need some more advice here. Following yesterday discussion i wrote some lines of code:

And now the real question, is there any C++ trick to avoid this double nesting?

facchinm commented 3 years ago

I'd go for option 2 (standalone objects) to avoid the second level of indirection as you perfectly noticed :slightly_smiling_face: Apart from this, I2C silk is different from all the other peripherals, so calling them I2C_X is perfectly fine

pennam commented 3 years ago

@facchinm @Rocketct i've updated the pr.

One more question about pin naming: for internal usage such as I2C class initialization should we go for I2C_0(PH_8,PH_7) or I2C_0(digitalPinToPinName(I2C_SDA_0), digitalPinToPinName(I2C_SDA_1))

facchinm commented 3 years ago

I think I2C_0(PH_8,PH_7) is ok, it spares the reader from one level of indirection :slightly_smiling_face: About PWMs, were are they connected? We know one HRTIMER was not avvailable via mbed APIs (@Rocketct can you provide the code we used for I don't remember which project?) but all the others should be ok

Rocketct commented 3 years ago

sure was the PMC, @pennam here (https://github.com/bcmi-labs/AutomationCarrierLibrary/blob/master/src/MachineControl.h#L196-L323) the PWM code based on HRTIMER, in the class i made, I've used the same naming so you should find it already aligned with the PWM ones, here the example if you need https://github.com/bcmi-labs/AutomationCarrierLibrary/blob/master/examples/Analog_Out/Analog_Out.ino.

Last but important how to instance, one of that object and the difference with respect the mbed::PWM https://github.com/bcmi-labs/AutomationCarrierLibrary/blob/master/src/MachineControl.h#L329-L384

pennam commented 3 years ago

Status update:

sebromero commented 3 years ago

Is there a reason why we want to adopt underscores for SPI and I2C while for UART we just append the number? e.g. UART1 vs I2C_0? Afaik in Arduino API we usually use camel-case without underscores. Same for functions. e.g. i2c_read() vs i2cRead(). https://www.arduino.cc/en/Reference/APIStyleGuide

pennam commented 3 years ago

We cannot use SPI0 and I2C0 because are already used as defines in ST mbed source files.

To access I2C functions i've reused Wire API so from sketch user needs to write:

Breakout.I2C_0.begin()
Breakout.I2C_0.beginTransmission()
Breakout.I2C_0.endTransmission()
Breakout.I2C_0.write()

The same for all other peripherals, i2c_read is an mbed API that needs to be fixed because has a bug, but there is no need to call it from sketch.

Rocketct commented 3 years ago

yes the macros without underscore are already defined on mbed's core so that name is the nearest as possible keeping the analogy with the silk

sebromero commented 3 years ago

Hmm, but we're using them as scoped variable names in BreakoutCarrierClass. Why can't we do Breakout.I2C0.begin()?

pennam commented 3 years ago

because I2C0 is a define and the preprocessor will replace it

sebromero commented 3 years ago

Ah, I see, actually I2C0 would work but I2C1 is defined :-D That's unfortunate.

pennam commented 3 years ago

For I2C issue i've submitted this patch https://github.com/ARMmbed/mbed-os/pull/14659 For UART flow control i'm working on a software implementation cause hw flow control is not available on that pins https://github.com/arduino/ArduinoCore-mbed/compare/master...pennam:sw_flow_control?expand=1

@facchinm should i wait/request a formal approval to merge this pr?

facchinm commented 3 years ago

LGTM!