Skip to content

New port for Infineon PSoC6 controllers #16705

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

jaenrig-ifx
Copy link
Contributor

Hi @mattytrentini @dpgeorge,

We have kept working on further enabling the PSoC6 controller in MPY. This pull request deprecates #15642.

Here a list of the features enabled:
machine classes:

  • ADC
  • ADCBlock
  • I2C. Master and slave mode.
  • I2S
  • Pin
  • PWM
  • RTC
  • SDCard
  • SPI. Master and slave mode.
  • Timer
  • UART
  • WDT

Additional modules:

  • network.WLAN
  • Virtual File System (LFS2 and FAT format)

PSoC6 evaluation boards available:

  • CY8CPROTO-062-4343W

  • CY8CPROTO-063-BLE

  • CY8CKIT-062S2-AI

  • Tests running in a hardware in the loop setup under ci/cd are provided for (at least) the main API functionalities of the enabled classes.

Looking forward to discuss the next steps regarding this PR with our teams.

The following modules are enabled:
- machine.ADC
- machine.ADCBlock
- machine.I2C
- machine.I2S
- machine.Pin
- machine.PWM
- machine.RTC
- machine.SDCard
- machine.SPI
- machine.Timer
- machine.UART
- machine.WDT
- network.WLAN
- os.vfs (LFS2 and FAT)

Evaluation boards initially integrated in the port:
- CY8CPROTO-062-4343W
- CY8CPROTO-063-BLE
- CY8CKIT-062S2-AI

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

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.53%. Comparing base (112f657) to head (fe81830).
Report is 147 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16705      +/-   ##
==========================================
- Coverage   98.59%   98.53%   -0.06%     
==========================================
  Files         167      169       +2     
  Lines       21599    21822     +223     
==========================================
+ Hits        21295    21502     +207     
- Misses        304      320      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

github-actions bot commented Feb 5, 2025

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +48 +0.012% PYBV10
     mimxrt:   +44 +0.012% TEENSY40
        rp2:   +56 +0.006% RPI_PICO_W
       samd:   +56 +0.021% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@jaenrig-ifx jaenrig-ifx changed the title Ports psoc6 pr new New port for Infineon PSoC6 controllers Feb 5, 2025
@IhorNehrutsa
Copy link
Contributor

Great work.

Modify micropython\tests\extmod_hardware\machine_pwm.py
to pass the test of machine.PWM().

Try to check code formatting by
micropython\tools\codeformat.py

@jaenrig-ifx
Copy link
Contributor Author

Great work.

Modify micropython\tests\extmod_hardware\machine_pwm.py to pass the test of machine.PWM().

Try to check code formatting by micropython\tools\codeformat.py

Thanks for you review @IhorNehrutsa !

You mean to add the psoc port and boards to that tests no? That we can do.

With the code formatting you mean after modifying the test?
We are running it in our ci and as pre-commit. The github action workflow for code formatting has passed for this PR.
Any error or failure we have missed or overlooked?


static void mp_machine_pwm_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
machine_pwm_obj_t *self = MP_OBJ_TO_PTR(self_in);
mp_printf(print, "frequency=%u duty_cycle=%f", self->fz, (double)self->duty);
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa Feb 5, 2025

Choose a reason for hiding this comment

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

mp_printf(print, "PWM(Pin(%s),freq=%u, duty_u16=%f", self->pin, self->fz, (double)self->duty);

As a test code something like:

>>> pwm = PWM(Pin(XXX), freq=5000, duty_u16=3272)
>>> pwm2 = eval(str(pwm))
>>> pwm
PWM(Pin(XXX), freq=5000, duty_u16=3272)
>>> pwm2
PWM(Pin(XXX), freq=5000, duty_u16=3272)

EDIT: Use similar test codes for other objects. Pin, ADC, UART etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a requirement/standard for machine classes that the human readable string representation of an object needs to match its constructor call?

In our case, this would use string pin labels (i.e. "P12_6") instead of pin objects (Pin(x)):

Virtual File System: mounted at '/' with LFS2 format in internal flash.

MicroPython 104dbf82c-dirty on 2025-02-06; CY8CPROTO-063-BLE with PSoC63
Type "help()" for more information.
>>> from machine import PWM
>>> pwm_string= "PWM(\"P12_6\", freq=5000, duty_u16=3272)"
>>> pwm =eval(str(pwm_string))
>>> pwm
PWM("P12_6",freq=5000, duty_u16=3272.000000)
>>>

Copy link
Contributor

Choose a reason for hiding this comment

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

Please take a look at the discussion in #4379.

Is this a requirement/standard for machine classes that the human-readable string representation of an object needs to match its constructor call?

IMHO: Yes. This allows save/restore, send/receive the objects.

Also:
PWM(Pin("P12_6"), freq=5000, duty_u16=3272) - is preferred.
PWM("P12_6", freq=5000, duty_u16=3272) - is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Then we have to review all the rest of machine classes, if we want to be consistent.

Regarding the Pin() vs label. We will stick to "the possible" as long as possible 😜, as the the Pin() object creation as in our port was rather artificial... due to the GPIO handling of the underlying PSOC6 apis for the different hw (machine) peripherals.
We thought on the impact in portability (across ports), but at the end of the day, for each port, the pin definition is specific... Therefore, we did not see the benefit on making our port code more intricated.
If this is a requirement then we have to refactor and redesign the machine classes... If we could skip that, at least for now, it would be great 🤞

@IhorNehrutsa
Copy link
Contributor

IhorNehrutsa commented Feb 6, 2025

You mean to add the psoc port and boards to that tests no?

Yes, add.

@jaenrig-ifx
Copy link
Contributor Author

Perfect. We will start working on those changes.
For now I will add them as additional commits. Before merging we can squash some of these changes if that is required. Is that okay?

@IhorNehrutsa is your review completed? Or just have started from the PWM module?

Comment on lines 84 to 87
void mod_wdt_deinit() {
cyhal_wdt_stop(&psoc6_wdt);
cyhal_wdt_free(&psoc6_wdt);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wdt_deinit was rejected here :
ESP32/machine_wdt.c: Add WDT.deinit() method. #6631

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The deinit is not exposed at user API level.

This function is called once the device is gets out of the REPL loop in the main.c (usually upon soft-reset):
https://github.com/Infineon/micropython/blob/ports-psoc6-pr-new/ports/psoc6/main.c#L225
As part of the macro MICROPY_PORT_DEINIT_FUNC:
https://github.com/Infineon/micropython/blob/ports-psoc6-pr-new/ports/psoc6/mpconfigport.h#L207-L208
Which calls them machine_deinit():
https://github.com/Infineon/micropython/blob/ports-psoc6-pr-new/ports/psoc6/modmachine.c#L124-L139

Its removal will prevent having a cleaned up post-reset operating system for the users, in which the watchdog could only be disabled through hard-reset.
I would need to understand how this is handled at other ports better, and the overall implications, before just getting rid of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't rush to delete; may wait for maintainers.

@IhorNehrutsa
Copy link
Contributor

Before merging we can squash some of these changes if that is required. Is that okay?

IMHO, okay. I have reviewed PWM and WDT.
Maintainers of the repo will review the full code.

self->duty_type = DUTY_U16;
}

if (args[ARG_duty_ns].u_int != VALUE_NOT_SET) {
Copy link
Contributor

Choose a reason for hiding this comment

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

else if (args[ARG_duty_ns].u_int != VALUE_NOT_SET) {

HERE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I go it :)

@jaenrig-ifx
Copy link
Contributor Author

You mean to add the psoc port and boards to that tests no?

Yes, add.

I tried, but we would need to implement machine.time_pulse_us() first.

Anyway, we have our own ci test for pmw:
https://github.com/Infineon/micropython/blob/ports-psoc6-pr-new/tests/ports/psoc6/board_ext_hw/single/pwm.py

In the long run I assume the goal is to generalize and reuse as much the common tests for all ports.
If this is a currently a long term goal, maybe we can add this is as improvement.

If this is a must for the PR to be approved, we can/should implement it. It should be rather quick unless we fail to achieve the pwm and time_pulse_us performance required for that test.

@IhorNehrutsa
Copy link
Contributor

Are all 183 files needed in PR?

@jaenrig-ifx
Copy link
Contributor Author

Are all 183 files needed in PR?

I would say so. We could argue about a few of them, but they are all used.

For example, the "mtb-libs" is used for integration of the toolchain and the dependencies of the PSOC6. We could make that a submodule, but it needs to be retrieved (initialized and updated as submodule) to build the port.

The tests are all running under our CI HIL (hardware-in-the-loop). But of course, they aren´t needed for building the PSOC6 MicroPython binary.

Some files under "tools/psoc6" might be only used by our fork ci hardware-in-the-loop workflow. If some of those are bothering, we could keep them in our fork only...

Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
@jaenrig-ifx
Copy link
Contributor Author

I won´t yet push to this branch until you consider the first review iteration done.
You can keep track of the changes already implemented here:
Infineon/micropython@ports-psoc6-pr-new...Infineon:micropython:pr-feedback

Comment on lines 77 to 83
static cy_rslt_t pwm_freq_duty_set(cyhal_pwm_t *pwm_obj, uint32_t fz, float duty_cycle) {
return cyhal_pwm_set_duty_cycle(pwm_obj, duty_cycle * 100, fz); // duty_cycle in percentage
}

static inline cy_rslt_t pwm_duty_set_ns(cyhal_pwm_t *pwm_obj, uint32_t fz, uint32_t pulse_width) {
return cyhal_pwm_set_period(pwm_obj, 1000000 / fz, pulse_width / 1000); // !# * --> /
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be more convenient to choose one of these functions cyhal_pwm_set_duty_cycle or cyhal_pwm_set_period and use only that one in both cases.

Also, it is possible to recalculate a duty at the last minute before calling cyhal_pwm_set_XXX and remove float and double in other places.

Choose a reason for hiding this comment

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

Understand it might be a bit confusing. But, taking reference from here: https://ifx-micropython.readthedocs.io/en/latest/library/machine.PWM.html#machine.PWM

  1. When duty_u16 is set, it is set as raw value and cyhal_pwm_set_duty_cycle allows to pass duty_cycle as percentage.
  2. When duty_ns is set, it is set as time in ns and cyhal_pwm_set_period allows to directly pass time.

It helps to reuse the low-level API's as much as intended functionality is provided through them. Does that explains?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
Anyhow, I think as long as possible is a good idea to restrict the conversion to one place. @IhorNehrutsa let us check if we can refactor without breaking any functionality :)

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 mentioned in the other comment:
Infineon@3c14c39

I have kept the 2 ways of setting freq/duty. But now abstracted and encapsulated under pwm_config().

@IhorNehrutsa
Copy link
Contributor

mp_raise_ValueError(MP_ERROR_TEXT("PWM duty should be specified in either ns or u16"));

There was a big discussion about what to do when duty_u16 or duty_ns are not specified.
Some ports do not start until both frequency and duty are specified, but do not raise an exception.
ESP32 port starts with a default 5000Hz & 50% duty.

@robert-hh
Copy link
Contributor

Some ports do not start until both frequency and duty are specified, but do not raise an exception.

Actually that is the common and intended behavior: not PWM signal unless both freq and duty cycle have been specified. There are no reasonable default values. The ESP32 does not yet meet that rule.

@dpgeorge dpgeorge added the ports Relates to multiple ports, or a new/proposed port label Feb 11, 2025
@IhorNehrutsa
Copy link
Contributor

I have no restriction about the

You can keep track of the changes already implemented here: Infineon/micropython@ports-psoc6-pr-new...Infineon:micropython:pr-feedback

You may merge

@IhorNehrutsa
Copy link
Contributor

You may interested in #16743

@jaenrig-ifx
Copy link
Contributor Author

mp_raise_ValueError(MP_ERROR_TEXT("PWM duty should be specified in either ns or u16"));

There was a big discussion about what to do when duty_u16 or duty_ns are not specified. Some ports do not start until both frequency and duty are specified, but do not raise an exception. ESP32 port starts with a default 5000Hz & 50% duty.

In the PR feedback changes we are currently making mandatory that freq and only one of the duty modes are set.

Any issue with this approach?

Something like in the esp32 port:
https://github.com/micropython/micropython/blob/master/ports/esp32/machine_pwm.c#L480-L482

@jaenrig-ifx
Copy link
Contributor Author

You may interested in #16743

Yes, let us put that in our backlog but with less prio.

@jaenrig-ifx
Copy link
Contributor Author

I have no restriction about the

You can keep track of the changes already implemented here: Infineon/micropython@ports-psoc6-pr-new...Infineon:micropython:pr-feedback

You may merge

If you approve the changes in Infineon@3c14c39
I will merge.

Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
Signed-off-by: enriquezgarc <enriquezgarcia.external@infineon.com>
@jaenrig-ifx
Copy link
Contributor Author

Feedback changes discussed merged now.

@IhorNehrutsa
Copy link
Contributor

Difficult to review due to the large number of files
https://github.com/micropython/micropython/pull/16705/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ports Relates to multiple ports, or a new/proposed port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants