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

esp32/machine_pin: Add mode and pull in machine_pin_print() as repr() function. #12293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IhorNehrutsa
Copy link
Contributor

Test code is:

from machine import Pin
pin = Pin(22, mode=Pin.IN, pull=Pin.PULL_UP)
pin

Output is:

Pin(22, mode=1, pull=2)

instead of

Pin(22)

The main advanteges:
This PR allows to serialize Pin object to str and restore the Pin object from the string:

pin2 = eval(str(pin))
pin2

Output is:

Pin(22, mode=1, pull=2)

@IhorNehrutsa IhorNehrutsa changed the title esp32/machine_pin:Add mode and pull in machine_pin_print() repr function. esp32/machine_pin: Add mode and pull in machine_pin_print() as repr() function. Aug 24, 2023
@IhorNehrutsa
Copy link
Contributor Author

@jimmo
Could you give your opinion about this PR?

@jimmo
Copy link
Member

jimmo commented Sep 1, 2023

Thanks @IhorNehrutsa -- I can definitely see that adding this information to the print output is useful for debugging, and making the pin serialisable is certainly a nice bonus.

However, there are two main concerns:

  1. We don't ensure that the pins can be eval'ed on other ports. For example, on rp2040:
>>> machine.Pin.board.D12
Pin(GPIO4, mode=ALT, pull=PULL_DOWN, alt=31)

It's quite useful (for debugging) that it prints the mode strings rather than the numbers too. (I guess you could in theory make this work with eval with the right combination of globals passed to eval).

  1. It's not a trivial decision to dedicate ~90 bytes (2 uint8_t * ~45 pins) of RAM just to tracking this state. 90 bytes isn't much, but I don't know if it justifies this? I was quite surprised to find that the IDF doesn't provide a way to extract this information from a pin, but that definitely seems to be the case in both the driver and HAL.

What if we turned machine_pin_obj_cfg_t into a bitfield (3 bits for gpio_mode_t, two bits for gpio_pull_mode_t). Then it would be only 45 bytes?

@IhorNehrutsa
Copy link
Contributor Author

@IhorNehrutsa
Copy link
Contributor Author

Function gpio_dump_io_configuration()
Dump IO configuration information to console.

Commit espressif/esp-idf@321f628
feat(gpio): add a dump API to dump IO configurations

@IhorNehrutsa
Copy link
Contributor Author

        bool pu, pd, ie, oe, od, slp_sel;
        uint32_t drv, fun_sel, sig_out;
        gpio_hal_get_io_config(gpio_context.gpio_hal, gpio_num, &pu, &pd, &ie, &oe, &od, &drv, &fun_sel, &sig_out, &slp_sel);

@IhorNehrutsa
Copy link
Contributor Author

feat(gpio): add a dump API to dump IO configurations (IDFGH-11366)
espressif/esp-idf#12511
Cherry pick from master to release/v5.1

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

@dpgeorge
Copy link
Member

Now that esp32 port uses IDF 5.2.2, are the required IDF functions available to retrieve the GPIO state?

Also, would be nice to print the string of the mode/pull, rather than just the integer (like the rp2 port does).

Copy link

Code size report:


Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>
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.

None yet

5 participants