Skip to content

Incorrect value for OUTPUT_OPEN_DRAIN in esp32-hal-gpio.h #8590

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

Closed
1 task done
TMSL opened this issue Aug 31, 2023 · 11 comments · Fixed by #8592
Closed
1 task done

Incorrect value for OUTPUT_OPEN_DRAIN in esp32-hal-gpio.h #8590

TMSL opened this issue Aug 31, 2023 · 11 comments · Fixed by #8592
Assignees
Labels
Area: Peripherals API Relates to peripheral's APIs. Status: Solved
Milestone

Comments

@TMSL
Copy link

TMSL commented Aug 31, 2023

Board

ESP32S3 Dev Kit C

Device Description

It appears OUTPUT_OPEN_DRAIN should be 0x13 instead of 0x12.
In ESP32-hal-gpio.h:

//GPIO FUNCTIONS
#define INPUT 0x01
// Changed OUTPUT from 0x02 to behave the same as Arduino pinMode(pin,OUTPUT)
// where you can read the state of pin even when it is set as OUTPUT
#define OUTPUT 0x03 <=== OUTPUT
#define PULLUP 0x04
#define INPUT_PULLUP 0x05
#define PULLDOWN 0x08
#define INPUT_PULLDOWN 0x09
#define OPEN_DRAIN 0x10 <=== OPEN_DRAIN
#define OUTPUT_OPEN_DRAIN 0x12 <=== should be 0x13
#define ANALOG 0xC0

Hardware Configuration

tested with pullup resistor on gpio 38

Version

v2.0.11

IDE Name

Arduino 2.2.0

Operating System

Linux Mint

Flash frequency

80 MHz

PSRAM enabled

yes

Upload speed

115200

Description

Setting "OUTPUT_OPEN_DRAIN (= 0x12)" in pinmode does not work, but using OUTPUT | OPEN_DRAIN ( = 0x13) does.

Sketch

#define OUTPIN 38
hw_timer_t *My_timer = NULL;

void IRAM_ATTR onTimer(){

digitalWrite(OUTPIN, !digitalRead(OUTPIN));
}

void setup() {

// FAILS:
pinMode(OUTPIN, OUTPUT_OPEN_DRAIN); // to get OPEN DRAIN use "OUTPUT | OPEN_DRAIN"

// create 1MHz counter by setting prescaler to 80 (ESP32 w/80MHz clock)
My_timer = timerBegin(0, 80, true);
timerAttachInterrupt(My_timer, &onTimer, true);
timerAlarmWrite(My_timer, 5, true); //1000000 counts @ 1MHz -> 1sec, 5->5usec
timerAlarmEnable(My_timer); //Enable the configured timer
}

void loop() {
}

Debug Message

none

Other Steps to Reproduce

none

I have checked existing issues, online documentation and the Troubleshooting Guide

  • I confirm I have checked existing issues, online documentation and Troubleshooting guide.
@TMSL TMSL added the Status: Awaiting triage Issue is waiting for triage label Aug 31, 2023
@P-R-O-C-H-Y
Copy link
Member

@TMSL I assume that it's right to be 0x12, as the pin should be only OUTPUT_OPEN_DRAIN and not INPUT_OUTPUT_OPEN_DRAIN as you use it. If you want to read the pin you have to call pinMode( PIN , (INPUT | OUTPUT_OPEN_DRAIN)); or pinMode( PIN , (OUTPUT | OPEN_DRAIN )); as you did. You can still use it without any issues.

@P-R-O-C-H-Y P-R-O-C-H-Y added Area: Peripherals API Relates to peripheral's APIs. and removed Status: Awaiting triage Issue is waiting for triage labels Aug 31, 2023
@P-R-O-C-H-Y
Copy link
Member

Closing this as duplicate of #7022.

@P-R-O-C-H-Y P-R-O-C-H-Y closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2023
@P-R-O-C-H-Y P-R-O-C-H-Y added the Resolution: Duplicate Issue is a duplicate of another issue label Aug 31, 2023
@TMSL
Copy link
Author

TMSL commented Aug 31, 2023

When the definition of OUTPUT was changed to 0x03 from 0x02 (per comment in .h file), it implicitly made OUTPUT set the pin to INPUT at the same time. (since INPUT is defined as 0x01 and the settings are treated as OR'd together).

But when the OUTPUT was changed to 0x03, the same automatically setting INPUT behavior was NOT added to the OUTPUT_OPEN_DRAIN definition. The problem with OUTPUT_OPEN_DRAIN being 0x12 is that it FAILS (because the code does not configure an output unless the value for INPUT is also SET). I.e. using pinmode with OUTPUT simultaneously sets INPUT, but using OUTPUT_OPEN_DRAIN in pinmode does not.

There are two ways get an OPEN DRAIN output to work: Either you need to use OUTPUT_OPEN_DRAIN | INPUT ( per issue #7022 ) or you need to use OUTPUT | OPEN_DRAIN (per #8590). But, again, unlike OUTPUT, using OUTPUT_OPEN_DRAIN (0x12) BY ITSELF DOES NOT WORK. Only something that produces 0x13 does.

@TMSL
Copy link
Author

TMSL commented Aug 31, 2023

P.S. - this is not a duplicate of #7022. #7022 wanted to set the pin to open drain output AND input at the same time. But I only wanted to set the pin to open drain output. BUT the 'bug' is that setting 0x12 FAILS unless I also set it as INPUT.

@P-R-O-C-H-Y
Copy link
Member

@TMSL Sorry about closing it that fast. My bad. I will try to do some tests today, so from what you wrote, the output_opendrain is not working at all? Or what exactly is not working?

I can open a PR with the fix you suggested today, if it's reasonable to get in into 2.0.12 release.

@TMSL
Copy link
Author

TMSL commented Sep 1, 2023

Thanks. Yes, when I used OUTPUT_OPEN_DRAIN alone I got no output signal from the GPIO pin at all, even though I had a pullup resistor connected. When I used OUTPUT it worked fine as a driven signal. When I changed the code to OUTPUT | OPEN_DRAIN it worked and I verifed that it was operating as OPEN_DRAIN by seeing the signal go away when I removed the pullup.
My test program was based on the RGB LED example. I viewed the oscillating signal on GPIO 48 using an oscilloscope.

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: In Progress ⚠️ Issue is in progress and removed Resolution: Duplicate Issue is a duplicate of another issue labels Sep 1, 2023
@P-R-O-C-H-Y P-R-O-C-H-Y self-assigned this Sep 1, 2023
@P-R-O-C-H-Y P-R-O-C-H-Y reopened this Sep 1, 2023
@P-R-O-C-H-Y
Copy link
Member

@TMSL Can you please post how you wired the pull-up and the LED to the GPIO? Thanks

@P-R-O-C-H-Y
Copy link
Member

@TMSL I have tested driving an LED without the timer, using just simple loop. It works the same when I used OUTPUT_OPEN_DRAIN and OUTPUT | OUTPUT_OPEN_DRAIN. Only difference is you cannot read the pin state as it uses 0x2 for only output instead of 0x3 as input_output to have output pin readable.

I got the LED flashing in both settings, only with 0x13 I am able to read the output pin.
Connected LED to the pin 38 and added an external pull-up to 3v3. LEDis flashing every 2 seconds and when I remove pull-up LEDis off.

SKETCH:

#define OUTPIN 38

void setup() {

Serial.begin(115200);
delay(10);

// PIN STATE READABLE, act as input_output
pinMode(OUTPIN, INPUT | OUTPUT_OPEN_DRAIN);

// PIN STATE NOT READABLE, act as output only
//pinMode(OUTPIN, OUTPUT_OPEN_DRAIN);
}
void loop() {

  Serial.println("Setting pin to HIGH");
  digitalWrite(OUTPIN, HIGH);
  Serial.println(digitalRead(OUTPIN));
  delay(2000);
  
  Serial.println("Setting pin to LOW");
  digitalWrite(OUTPIN, LOW);
  Serial.println(digitalRead(OUTPIN));
  delay(2000);
}

LOG using INPUT | OUTPUT_OPEN_DRAIN:

Setting pin to HIGH
1
Setting pin to LOW
0
Setting pin to HIGH
1
Setting pin to LOW
0

LOG using OUTPUT_OPEN_DRAIN:

Setting pin to HIGH
0
Setting pin to LOW
0
Setting pin to HIGH
0
Setting pin to LOW
0

So from that the open drain works fine when used the OUTPUT_OPEN_DRAIN. Only you are not able to read the pins state.
That is the same issue as #7022.

@TMSL
Copy link
Author

TMSL commented Sep 1, 2023

THANKS AGAIN!! I see my issue. I had copied code from an example that toggled the GPIO using: DigitalWrite(OUTPIN, !DigitalRead(OUTPIN)); I also see that Arduino (e.g. for Atmega328p) only defines INPUT, OUTPUT, and INPUT_PULLUP in Arduino.h. I believe all other GPIO functions in esp32-hal-gpio.h are ESP-specific.

I'M NOT REQUESTING ANY MORE CHANGES or re-opening of this issue, but just noting my understanding of the present bit definitions:

//GPIO FUNCTIONS
#define INPUT 0x01 // Arduino equivalent function. Enables input.
// Changed OUTPUT from 0x02 to behave the same as Arduino pinMode(pin,OUTPUT)
// where you can read the state of pin even when it is set as OUTPUT
#define OUTPUT 0x03 //Arduino equivalent function: enables input + driven output
#define PULLUP 0x04 //ESP: Sets pullup only. Does not enable input or output.
#define INPUT_PULLUP 0x05 //Arduino equivalent function: enables input with pullup
#define PULLDOWN 0x08 //ESP: Sets pulldown only. Does not enable input or output.
#define INPUT_PULLDOWN 0x09 //ESP: enables input with pulldown
#define OPEN_DRAIN 0x10 //ESP: Sets open drain only. Does not enable input or output.
#define OUTPUT_OPEN_DRAIN 0x13 //ESP: enables input + open drain output. Similar to Arduino OUTPUT

I think another ESP-specific definitions are also possible:
OUTPUT_E 0x02 //ESP-specific: Enables output but not input.
OUTPUT_E_OPEN_DRAIN 0x12 //ESP-specific: Enables open drain output, but not input.

For what it's worth, 0x01, 0x02, 0x04, 0x08 and 0x10 appear to be defined for use as bits to be OR'd together, where:

//FUNCTIONS:
#define INPUT 0x01 //Arduino equiv.
#define OUTPUT 0x03 //Arduino equiv.
#define INPUT_PULLUP 0x05 //Arduino equiv.
#define INPUT_PULLDOWN 0x09 //ESP
#define OUTPUT_OPEN_DRAIN 0x13 //ESP

// Individual ENABLE (ESP):
// E.g. OUTPUT = OUTPUT_E | INPUT, INPUT_PULLUP = INPUT | PULLUP_E
// OUTPUT_OPEN_DRAIN = OUTPUT | OPEN_DRAIN_E, etc.
#define INPUT _E 0x01 // redundant with INPUT, but included for symmetry
#define OUTPUT_E 0x02
#define PULLUP_E 0x04
#define PULLDOWN_E 0x08
#define OPEN_DRAIN_E 0x10

@TMSL
Copy link
Author

TMSL commented Sep 1, 2023

I think my last comment was hard to read. This should be better:

//GPIO FUNCTIONS
#define INPUT             0x01 // Arduino equivalent function. Enables input.
// Changed OUTPUT from 0x02 to behave the same as Arduino pinMode(pin,OUTPUT)
// where you can read the state of pin even when it is set as OUTPUT
#define OUTPUT            0x03 //Arduino equivalent function: enables input + driven output
#define PULLUP            0x04 //ESP: Sets pullup only. Does not enable input or output.
#define INPUT_PULLUP      0x05 //Arduino equivalent function: enables input with pullup
#define PULLDOWN          0x08 //ESP: Sets pulldown only. Does not enable input or output.
#define INPUT_PULLDOWN    0x09 //ESP: enables input with pulldown
#define OPEN_DRAIN        0x10 //ESP: Sets open drain only. Does not enable input or output.
#define OUTPUT_OPEN_DRAIN 0x13 //ESP: enables input + open drain output. Similar to Arduino OUTPUT

For what it's worth, 0x01, 0x02, 0x04, 0x08 and 0x10 appear to be defined for use as bits to be OR'd together, where:

//GPIO FUNCTIONS:
#define INPUT             0x01 //Arduino equiv.
#define OUTPUT            0x03 //Arduino equiv.: input + driven output
#define INPUT_PULLUP      0x05 //Arduino equiv.:input + pullup
#define INPUT_PULLDOWN    0x09 //ESP: input + pulldown
#define OUTPUT_OPEN_DRAIN 0x13 //ESP: input + open drain output

// GPIO Individual ENABLES (ESP):
// E.g. OUTPUT = OUTPUT_E | INPUT, INPUT_PULLUP = INPUT | PULLUP_E
// OUTPUT_OPEN_DRAIN = OUTPUT | OPEN_DRAIN_E, etc.
#define INPUT _E          0x01 // redundant with INPUT, but included for symmetry
#define OUTPUT_E          0x02
#define PULLUP_E          0x04
#define PULLDOWN_E        0x08
#define OPEN_DRAIN_E      0x10

@TMSL
Copy link
Author

TMSL commented Sep 1, 2023

final version:

//GPIO FUNCTIONS:
#define INPUT             0x01 //Arduino equiv.
#define OUTPUT            0x03 //Arduino equiv.: input + driven output
#define INPUT_PULLUP      0x05 //Arduino equiv.:input + pullup
#define INPUT_PULLDOWN    0x09 //ESP: input + pulldown
#define OUTPUT_OPEN_DRAIN 0x13 //ESP: input + open drain output

// GPIO Individual ENABLES (ESP):
// E.g. OUTPUT = OUTPUT_E | INPUT, INPUT_PULLUP = INPUT | PULLUP
// OUTPUT_OPEN_DRAIN = OUTPUT | OPEN_DRAIN, etc.
#define INPUT_E           0x01 // redundant with INPUT, but included for symmetry
#define OUTPUT_E          0x02
#define PULLUP            0x04
#define PULLDOWN          0x08
#define OPEN_DRAIN        0x10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs. Status: Solved
Projects
Development

Successfully merging a pull request may close this issue.

3 participants