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

Expand uart invert feature to ESP8266 #1727

Merged
merged 2 commits into from
Dec 1, 2021
Merged

Conversation

Lewn
Copy link
Contributor

@Lewn Lewn commented Apr 26, 2021

What does this implement/fix?

Expands the invert option added for the ESP32 in #1586 to the ESP8266.

Changes the invert option from #1586 to use the more idiomatic configuration using the full pin scheme. It also implements this feature for the ESP8266.

As the esp8266 arduino library only implements this for UART0, a combination of invert and tx_pin: GPIO2 will fall back to using software UART.

To also support different pin combinations, a the inverting behaviour is also implemented for ESP8266SoftwareSerial.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (this will require users to update their yaml configuration files to keep working) (on request of Otto, see below)

Related issue or feature (if applicable): N/A

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#1117

Test Environment

  • ESP32
  • ESP8266
  • Windows
  • Mac OS
  • Linux (Home Assistant on Home Assistant OS)
  • ADALM2000 logic analyzer/signal generator to verify incoming and outgoing signals

Example entry for config.yaml:

# Example config.yaml
uart:
  # UART0
  - id: uart0
    tx_pin: 
      number: GPIO1
      inverted: yes
    rx_pin: GPIO3
    baud_rate: 115200
  - id: not_uart1
    tx_pin: 
  # SoftwareSerial
  - id: sw
    tx_pin: GPIO5
    rx_pin: 
      number: GPIO4
      inverted: yes
    baud_rate: 115200

Explain your changes

I needed to communicate over inverted lines, and this was only supported for the ESP32, I decided to implement it for the ESP8266 for my own needs. I think it might benefit others as well.

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as its been labeled with an integration (uart) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@glmnet glmnet left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM docs too.

Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

Wait... why is there even an invert option?

Invert should already be possible just by using the invert: property of the tx_pin/rx_pin (or if not, then that mechanism should be used for inverting line levels).

@Lewn Lewn force-pushed the UART-invert-for-esp8266 branch from 4b8553a to 1112ac8 Compare May 4, 2021 14:25
@glmnet
Copy link
Member

glmnet commented May 4, 2021

Wait... why is there even an invert option?

Invert should already be possible just by using the invert: property of the tx_pin/rx_pin (or if not, then that mechanism should be used for inverting line levels).

we should then rollback invert: already made for esp32, also it's not clear if the invert is valid / supported only for one of the RX/TX lines exclusively, otherwise a validation should be enforced.

@Mynasru
Copy link

Mynasru commented May 5, 2021

Wait... why is there even an invert option?

Invert should already be possible just by using the invert: property of the tx_pin/rx_pin (or if not, then that mechanism should be used for inverting line levels).

First of all, thanks for your great work Otto (& others)!
I worked on the uart invert option for ESP32, and we put it as an option for the uart component instead of a pin option because it is then the same way as the Serial.begin() option underneath (it just exposes it to ESPHome).
If this function is going to be revised, maybe extend it to all uart pins like in the Espresiff-idf + software serial + ESP8266 implementation.

@glmnet
Copy link
Member

glmnet commented May 10, 2021

Adding the invert option to pin might take the feature just working (at least on the software serial tx) so I believe Otto's idea is the way to go. May be we can fix this pr to do that and then the esp32 fix in another pr.

Sounds good?

@jesserockz jesserockz added this to In Review in ESPHome Dev May 19, 2021
@Lewn Lewn force-pushed the UART-invert-for-esp8266 branch 2 times, most recently from c9741fa to f3904cc Compare May 30, 2021 13:39
@probot-esphome probot-esphome bot removed the small-pr label May 30, 2021
@Lewn
Copy link
Contributor Author

Lewn commented May 30, 2021

Updated to use the pin scheme as requested by Otto. This indeed seems a more natural way of handeling things. Right now it is not yet supported to only invert RX or TX on the ESP32, but this should be an edge case anyway.

Let me know what you think.

@Lewn Lewn force-pushed the UART-invert-for-esp8266 branch from 10f6e31 to e513c55 Compare May 31, 2021 16:55
@Lewn
Copy link
Contributor Author

Lewn commented May 31, 2021

Updated for backward compatibility, I'm not sure if this is the correct way of handling it, I'm open to suggestions.

@glmnet
Copy link
Member

glmnet commented Jun 1, 2021

Updated for backward compatibility, I'm not sure if this is the correct way of handling it, I'm open to suggestions.

No, just do the cv.Invalid and deprecate the option right away. It's just a configuration change.

@Lewn Lewn force-pushed the UART-invert-for-esp8266 branch from e513c55 to 9747ee5 Compare June 1, 2021 11:29
@Lewn
Copy link
Contributor Author

Lewn commented Jun 1, 2021

No, just do the cv.Invalid and deprecate the option right away. It's just a configuration change.

Updated

@mneuron
Copy link

mneuron commented Nov 6, 2021

Any progress because with the current uart component only esp32 has invert. Would be nice if esp8266 has also the possibility for hardware invert when using DSMR ( which needs inverted RX)

@mmakaay
Copy link
Member

mmakaay commented Nov 6, 2021

Would be nice if esp8266 has also the possibility for hardware invert when using DSMR

One problem with that is that we are seeing that DSMR with software UART can be very unstable, up to the level that not a single telegram can be read successfully because of missing or garbled bytes.
I have tried to create a signal inverter option for ESP8266 too, but I dropped that project because of the issues that I had. Using a few components to do the inversion in hardware, and making sure that the ESP8266 can then use a hardware UART made a really big difference.

@Lewn
Copy link
Contributor Author

Lewn commented Nov 9, 2021

From my point of view, this should be able to get merged, I will rebase to the latest dev branch later today.
Since I made this PR, there has been some changes that need to be incorporated into this PR.

I also needed this for DSMR, and it has been running since the opening of this PR without issues, so the HW invert definitly works.

@jesserockz
Copy link
Member

@Lewn think you can update the branch/PR to be ready for beta release tomorrow?

@Lewn Lewn force-pushed the UART-invert-for-esp8266 branch 2 times, most recently from 60f43f5 to ae105db Compare November 10, 2021 18:48
@Lewn
Copy link
Contributor Author

Lewn commented Nov 10, 2021

Updated the code, it should work now, but I cannot test it on my physical setup right now. Please review with care before merging.

For reference, the branch that ran successfully for months on my DSMR node: https://github.com/Lewn/esphome/tree/UART-invert-refactor

@mneuron
Copy link

mneuron commented Nov 16, 2021

Updated the code, it should work now, but I cannot test it on my physical setup right now. Please review with care before merging.

For reference, the branch that ran successfully for months on my DSMR node: https://github.com/Lewn/esphome/tree/UART-invert-refactor

Using your UART-invert-for-esp8266 branch as an external component I can confirm a succesful compile.
My wemos-d1 build DSMR reader works with inverted -RX on hardware UART without errors! No more transistor for signal inversion needed, thanx!
@jesserockz - think you can merge for next beta?

@endor-force
Copy link

This PR looks like it would solve feature request: #1727

Copy link
Member

@oxan oxan left a comment

Choose a reason for hiding this comment

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

Sorry that this has been such a bumpy ride and that I'm having even more comments now, but given the importance of the UART component, I'd like to get this right.

Is there anyone that can test the software serial?

esphome/components/uart/uart_component_esp8266.cpp Outdated Show resolved Hide resolved
esphome/components/uart/uart_component_esp8266.cpp Outdated Show resolved Hide resolved
esphome/components/uart/uart_component_esp8266.cpp Outdated Show resolved Hide resolved
@Lewn
Copy link
Contributor Author

Lewn commented Dec 1, 2021

Reverted the softwareserial changes, as they are already working today, rebased to dev.

ESPHome Dev automation moved this from In Review to Reviewer Approved Dec 1, 2021
@oxan oxan merged commit 11330af into esphome:dev Dec 1, 2021
ESPHome Dev automation moved this from Reviewer Approved to Done Dec 1, 2021
@jesserockz jesserockz mentioned this pull request Dec 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2021
ESPHome Dev automation moved this from Done to Reviewer Approved Sep 23, 2023
ESPHome Dev automation moved this from Reviewer Approved to Done Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants