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

Dev i2c stm32f4hal #3238

Merged
merged 14 commits into from
Nov 16, 2016
Merged

Dev i2c stm32f4hal #3238

merged 14 commits into from
Nov 16, 2016

Conversation

LMESTM
Copy link
Contributor

@LMESTM LMESTM commented Nov 9, 2016

Description

This PR contains rework of F4 I2C driver to move most of the drivers functions to call STM32 cube HAL API instead of direct registers access .
Because this API is common to all STM32 families, this is a first step towards merging the various (x9) i2c_api.c files into a common file.
During the rework, other functionalities have been added: like usage of Interrupts as well in sync mode (not visible fro MBED API user), reset of IP in case of error, timeout calculated based on SystemCoreClock and I2C frequency

Status

READY

  • Tests report

IKS01A1 Hello world has been tested succesfully.

Also non regression tests results available here:
image

Instead of direct registers access, let's use HAL API.
This makes the code more generic accross STM32 families.
No need to store the init status of each IP.
Init can be called again in case we try to recover.
so that it can be used as well in sync mode
With this commit we define I2C irq handlers that can be used by the driver
in sync mode. This also provides a mecanism for enabling and/or disabling
these handlers

Those handlers will be superseded by MBED ones in case of async mode usage.
With this new implementation, the slave use the Interrupt
to be notified of a request from master, instead of
accessing to registers continuously.

This has 2 main advantages:
- this shall improve performances overall and allows for sleep
time in the future
- this also removes some direct registers access from this
layer of code and makes it more generic among families
The timeout values are based on for loops and therefore should depend
on the core frequency and the I2C interface frequency.

This patch introduces this computation and base the timeout on the time
it should take to send a byte over the I2C interface. When sending a
number of bytes, this value can also be used.

In the loops, the timeout should also be decreased before the while
condition so that its value is 0 in case the timeout elapsed and this
can be treated as an error.
This is to avoid an IP / bus deadlock.

This requires to store scl and sda in order to call the init function.
With this new implementation, as in slave implementaiton, we use the
interrupts instead of accessing to registers continuously.

This has 2 main advantages:
- this shall improve performances overall and allows for sleep
time in the future
- this also removes some direct registers access from this
layer of code and makes it more generic among families
in case of 2 consecutives calls to HAL_I2C_Master_Sequential_Receive_IT
with the Xfer mode I2C_FIRST_AND_LAST_FRAME, the second trasnfer does
not start at all.

It seems this is because the previous state is maintained as I2C_STATE_MASTER_BUSY_RX
and therefore the START condition will not be generated
The cbmaster_done function is a callback which will be called from
the asynch I2C interrupt handler. Calling to printf from this context
sometimes lead to missing interrupts on the slave side. This was at least
encountered on STM32F3 MCUs.
@@ -84,17 +84,28 @@ struct spi_s {
};

struct i2c_s {
/* The 1st 2 members I2CName i2c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curiousity: what's the reasoning for this comment, should it be mentioned ??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a requirement for get_i2c_obj function to work - if anyone changes this order, this would break the mechanism to get a pointer to the i2c_s object when having a pointer to the contained handle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, it was not clear from the comment why there's that requirement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - I'll update the comment to be more precise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK let me know once done . I'll go through the files once again, trigger CI afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - I updated the comment

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 10, 2016

/morph test


#if DEVICE_I2C_ASYNCH
#define I2C_S(obj) (struct i2c_s *) (&((obj)->i2c))
#else
#define I2C_S(obj) (struct i2c_s *) (obj)
#endif

/* could be defined at family level */
#define I2C_NUM (5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes all F4 have 5 I2C peripherals. Is this correct or will be updated to have the correct number per target/subfaamily?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see it now, this will be adapted on a per family basis (all F4 will have to handle up to 5 I2C)

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1044

All builds and test passed!

As reported during review, this was not understandable as it is.
the get_i2c_obj allows to get a pointer to i2c_s struct from the
handle pointer. This therefore makes a hard-coded assumption
about the struct itself
@sg-
Copy link
Contributor

sg- commented Nov 15, 2016

@marhil01 Can you check pr-head CI failure?

@0xc0170 0xc0170 merged commit 5750f31 into ARMmbed:master Nov 16, 2016
aisair pushed a commit to aisair/mbed that referenced this pull request Apr 30, 2024
Ports for Upcoming Targets


Fixes and Changes

3127: Fixed the issue about push/pop of VFP register. ARMmbed/mbed-os#3127
3176: Modifying micro:bit pin names to mirror micro:bit edge connector ARMmbed/mbed-os#3176
3160: Fix wrong index at LPC43xx tx end ring assignment ARMmbed/mbed-os#3160
3217: Add I2C_ASYNCH capability for DISCO_F469NI ARMmbed/mbed-os#3217
3211: [NUC472/M453] Support single UART shared by multiple serial objects and other updates ARMmbed/mbed-os#3211
3198: NUCLEO_F410RB: Add I2C_ASYNCH capability ARMmbed/mbed-os#3198
3194: Update K64 sdk drivers ARMmbed/mbed-os#3194
3159: User trim values for NCS36510 ARMmbed/mbed-os#3159
3243: Fix make exporters compilation ARMmbed/mbed-os#3243
3231: STM32F3: DISCO_F303VC - Add missing UART and ADC pin muxing options ARMmbed/mbed-os#3231
3233: K20xx Calculate PWM clock relative to bus clock ARMmbed/mbed-os#3233
3237: Added back USART 6 pins ARMmbed/mbed-os#3237
3253: Fix default polarity on LPC43XX PWM driver ARMmbed/mbed-os#3253
3238: Dev i2c stm32f4hal ARMmbed/mbed-os#3238
3251: Dev stm32l0 cube v1.7.0 ARMmbed/mbed-os#3251
3252: [NORDIC - NRF51 - MBED 2] Fix non handled RTC IRQ ARMmbed/mbed-os#3252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants