Skip to content

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

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

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

Conversation

IhorNehrutsa
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa commented Aug 23, 2023

Summary
This PR adds mode, pull, drive parameters to the Pin repr function.

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

pin1 = Pin(27, mode=Pin.IN, pull=Pin.PULL_DOWN)
pin2 = eval(str(pin))
pin2

Output is:

Pin(27, mode=Pin.IN, pull=Pin.PULL_DOWN)

Testing
Test code is:

from time import sleep
from machine import Pin

PIN = 27 # 5 #

pin = Pin(PIN, mode=Pin.IN, pull=Pin.PULL_DOWN)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.IN, pull=Pin.PULL_UP)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.IN, pull=None)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.OUT, value=0)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.OUT, value=1, drive=Pin.DRIVE_1)
sleep(2)
print(pin, pin())

Output is:

Pin(27, mode=Pin.IN, pull=Pin.PULL_DOWN) 0
Pin(27, mode=Pin.IN, pull=Pin.PULL_UP) 0
Pin(27, mode=Pin.IN) 0
Pin(27, mode=Pin.OUT) 0
Pin(27, mode=Pin.OUT, drive=Pin.DRIVE_1) 1

This PR requires:
esp-idf commit espressif/esp-idf@4535c27
esp32: IDF v.5.5 compile. #16565

See also comment
espressif/esp-idf#14923 (comment)

@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:


@IhorNehrutsa
Copy link
Contributor Author

Test code is:

from time import sleep
from machine import Pin

PIN = 27 # 5 #

pin = Pin(PIN, mode=Pin.IN, pull=Pin.PULL_DOWN)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.IN, pull=Pin.PULL_UP)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.IN, pull=None)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.OUT, value=0)
sleep(2)
print(pin, pin())

pin = Pin(PIN, mode=Pin.OUT, value=1, drive=Pin.DRIVE_1)
sleep(2)
print(pin, pin())

The output is:

Pin(27, mode=Pin.IN, pull=Pin.PULL_DOWN) 0
Pin(27, mode=Pin.IN, pull=Pin.PULL_UP) 0
Pin(27, mode=Pin.IN) 0
Pin(27, mode=Pin.OUT) 0
Pin(27, mode=Pin.OUT, drive=Pin.DRIVE_1) 1

This PR requires in esp-idf
GPIO: Add gpio_get_io_config() and xxx_is_enabled(). (IDFGH-14117)

@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Jan 10, 2025

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).

This PR requires:
esp-idf commit espressif/esp-idf@4535c27
esp32: IDF v.5.5 compile. #16565

See also comment
espressif/esp-idf#14923 (comment)

@projectgus projectgus marked this pull request as draft January 14, 2025 05:46
@projectgus
Copy link
Contributor

Thanks @IhorNehrutsa for persisting with this PR.

I've marked it as a draft for now, because we won't be able to review and merge it until there's a release version of ESP-IDF v5.5 that MicroPython supports. When this happens, please feel free to rebase and click "Ready for review".

(One thing to note, it will be a very long time before we drop support for all older ESP-IDF, so this PR will probably need some #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 5, 0) guards around it.)

Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>
@IhorNehrutsa
Copy link
Contributor Author

#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 5, 0) guards around it.

Done.

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.

5 participants