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

Breakout I2C UART SPI AnalogRead AnalogWrite #9

Merged
merged 27 commits into from
May 19, 2021

Conversation

pennam
Copy link
Collaborator

@pennam pennam commented Mar 31, 2021

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

  • Going for nesting such as Breakout.I2C_0.begin() I think is not possible to declare objects into external file Objects.cpp cause they need to be inside BreakoutCarrierClass
  • I2CX are used as define into STM32Hal so we need to use I2C_X
  • PwmOut.h is not in the core so we cannot inherit from PwmOut object.
  • For better readability i prefer to group objects in sub classes like BreakoutI2CClass, BreakoutUARTClass, BreakoutPWMClass , but this adds another nesting level and the result to the user will be Breakout.I2C.BUS_0.begin()

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

@facchinm
Copy link
Collaborator

facchinm commented Apr 1, 2021

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

@pennam
Copy link
Collaborator Author

pennam commented Apr 7, 2021

@facchinm @Rocketct i've updated the pr.

  • I2C is working
  • analogWrite on PWM pins works, but not on PWM3 (mbed crash) and not consistently on PWM4
  • Need to test UARTS

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
Copy link
Collaborator

facchinm commented Apr 7, 2021

I think I2C_0(PH_8,PH_7) is ok, it spares the reader from one level of indirection 🙂
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
Copy link
Collaborator

Rocketct commented Apr 7, 2021

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
Copy link
Collaborator Author

pennam commented Apr 13, 2021

Status update:

  • I2C working but .available() is returning always 72 because of mbed i2c_read() bug
  • analogRead works
  • analogWrite works but PWM4 and PWM8 cannot work together because are sharing the same timer channel (Confirmed). Minimum frequency increased to 770Hz due to HRTIM.
  • SPI works, name will be SPI_0 as for I2C (SPI0 is an ST define)
  • SDCARD works (on my breakout R1 was missing)
  • UART works, flow control is disabled because it causes mbed crash
  • PDM works

@pennam pennam changed the title [WIP] I2C UART PWM proposal Breakout I2C UART SPI AnalogRead AnalogWrite Apr 20, 2021
@sebromero
Copy link
Contributor

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
Copy link
Collaborator Author

pennam commented Apr 20, 2021

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
Copy link
Collaborator

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
Copy link
Contributor

sebromero commented Apr 20, 2021

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

@pennam
Copy link
Collaborator Author

pennam commented Apr 20, 2021

because I2C0 is a define and the preprocessor will replace it

@sebromero
Copy link
Contributor

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

@pennam pennam mentioned this pull request Apr 22, 2021
@pennam pennam requested a review from Rocketct May 13, 2021 07:56
@pennam pennam requested a review from facchinm May 17, 2021 07:09
@pennam
Copy link
Collaborator Author

pennam commented May 19, 2021

For I2C issue i've submitted this patch ARMmbed/mbed-os#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
Copy link
Collaborator

LGTM!

@pennam pennam merged commit 792c092 into arduino-libraries:main May 19, 2021
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.

4 participants