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

Machine i2c deinit #96

Merged
merged 7 commits into from
Oct 17, 2023
Merged

Conversation

jaenrig-ifx
Copy link
Member

By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md
--- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

CONTRIBUTING.md also tells you what to expect in the PR process.

To be reviewed after #95.

Added deinitialization function to i2c module. This is directly added to the extmod.

Docs amendments will be done after the complete machine module is refactored for the pin_phy.

Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

STATIC const mp_machine_i2c_p_t machine_i2c_p = {
.deinit = machine_i2c_deinit,
Copy link
Member

Choose a reason for hiding this comment

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

Not completely sure if this is needed. So far I understand, this is to point to our version of implementation of function, if there is already one implemented in other module (for ex. in extmod/i2c). And I think there is not deinit in extmod. But I might be wrong and feel free to correct.

Copy link
Member Author

@jaenrig-ifx jaenrig-ifx Oct 17, 2023

Choose a reason for hiding this comment

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

Correct. That was not declared in the extmod. And I added it.
How can the i2c then be deinited? I wonder how other ports handle softreset: if there is no deinit(), after a softreset the i2c is still configured in whatever mode? If we want to reuse? we reinitiaze? with no impact?

As discusssed, I don´t yet understand how the garbage collector handles destructing all objects... but I don´t see (yet) the connection to hardware peripheral deinitialization. That has to be handled somewhere (freeing before initing() all the time?)...

@@ -189,3 +218,11 @@ MP_DEFINE_CONST_OBJ_TYPE(
protocol, &machine_i2c_p,
locals_dict, &mp_machine_i2c_locals_dict
);

void mod_i2c_deinit() {
Copy link
Member

Choose a reason for hiding this comment

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

Also handles for softI2C?

Copy link
Member Author

@jaenrig-ifx jaenrig-ifx Oct 17, 2023

Choose a reason for hiding this comment

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

Nop. The I2CSoft is completely handled by the extmod... (and relying on mp_hal functions). I also wonder if something there are wonder. I understand this works using pins, but I don´t see the gpio initialization being handled anywhere.
Was the softi2c functionally tested?

Choose a reason for hiding this comment

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

GPIO initialization for the softI2C is handled inside mphalport.c using cyhal_gpio_configure( ) function

Copy link
Member Author

@jaenrig-ifx jaenrig-ifx Oct 17, 2023

Choose a reason for hiding this comment

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

Mmm okay, I think then we have to extend that to initialize that pin if not inited already.

#define DEFAULT_I2C_FREQ (400000)
#define MICROPY_HW_I2C_SCL (CYBSP_I2C_SCL)

Choose a reason for hiding this comment

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

Initially, we had

from machine import I2C
i2c = I2C(0)

This will do the i2c init with default pins. So that is removed with this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I removed that. There is no such thing as "default" pins unless we provide pre-instantiated objects to those default pins allocation... and that has to be documented in a pinout diagram or table.
For multiple board, this is harder to maintain. But we can have such default allocation, then that comes with a nice pin out diagram and we have to add those to the code. But then the user can happily call i2c = I2C(0) for any board.
In the end, the user needs to know the pins, so we can skip the magic of not providing any (at least for now).

Choose a reason for hiding this comment

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

okay. sounds good

Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
@jaenrig-ifx jaenrig-ifx merged commit ddd748d into machine_pin_adc_deinit Oct 17, 2023
18 of 20 checks passed
@jaenrig-ifx jaenrig-ifx deleted the machine_i2c_deinit branch October 17, 2023 11:39
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.

3 participants