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

Exposes load_settings to UARTComponent class #5920

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Conversation

edwardtfn
Copy link
Contributor

What does this implement/fix?

This will expose load_settings (introduced on #5909) to UARTComponent class and any component using that class. I'm changing this as I wanna Nextion component to be able to change it's baud rate on the communication with the display without the need to create temporary UART components to handle it.

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)
  • Other

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

Pull request in esphome-docs with documentation (if applicable): N/A

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

N/A

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:

This will expose `load_settings` (introduced on esphome#5909) to `UARTComponent` class and any component using that class.
I'm changing this as I wanna Nextion component to be able to change it's baud rate on the communication with the display without the need to create temporary UART components to handle it.
@edwardtfn edwardtfn marked this pull request as ready for review December 13, 2023 15:19
@edwardtfn edwardtfn requested a review from a team as a code owner December 13, 2023 15:19
@probot-esphome
Copy link

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

@jesserockz jesserockz merged commit 81aa48a into esphome:dev Dec 13, 2023
44 checks passed
@jesserockz jesserockz mentioned this pull request Dec 13, 2023
@SeByDocKy
Copy link
Contributor

SeByDocKy commented Dec 14, 2023

Be aware with this PR, i got now compilation Error ..... was a breaking change ....

src/main.cpp: In function 'void setup()':
src/main.cpp:291:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_00 = new wk2132_i2c::WK2132Channel();
                                           ^
In file included from src/esphome.h:82,
                 from src/main.cpp:3:
src/esphome/components/wk2132_i2c/wk2132_i2c.h:532:7: note:   because the following virtual functions are pure within 'esphome::wk2132_i2c::WK2132Channel':
 class WK2132Channel : public uart::UARTComponent {
       ^~~~~~~~~~~~~
In file included from src/esphome/components/uart/uart.h:7,
                 from src/esphome/components/jk_modbus/jk_modbus.h:4,
                 from src/esphome/components/jk_bms/jk_bms.h:8,
                 from src/esphome.h:29,
                 from src/main.cpp:3:
src/esphome/components/uart/uart_component.h:66:16: note: 	'virtual void esphome::uart::UARTComponent::load_settings()'
   virtual void load_settings() = 0;
                ^~~~~~~~~~~~~
src/esphome/components/uart/uart_component.h:67:16: note: 	'virtual void esphome::uart::UARTComponent::load_settings(bool)'
   virtual void load_settings(bool dump_config) = 0;
                ^~~~~~~~~~~~~
src/main.cpp:298:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_01 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:330:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_10 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:337:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_11 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:369:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_20 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:376:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_21 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:408:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel'
   uart_30 = new wk2132_i2c::WK2132Channel();
                                           ^
src/main.cpp:415:43: error: invalid new-expression of abstract class type 'esphome::wk2132_i2c::WK2132Channel```

@edwardtfn
Copy link
Contributor Author

I will take a look at that later.
Are you running under esp32 or esp8266?

@SeByDocKy
Copy link
Contributor

I will take a look at that later. Are you running under esp32 or esp8266?

ESP32 ...

@DrCoolzic
Copy link
Contributor

DrCoolzic commented Dec 14, 2023

Yes I confirm breaking change for no good reason. It is ok that you add these fonctions as virtuals but NOT pure virtuals.....

@DrCoolzic
Copy link
Contributor

I will take a look at that later. Are you running under esp32 or esp8266?

ESP32 ...

I cant do anything now because i am on a Phone but i will make a temp fix for you

@DrCoolzic
Copy link
Contributor

DrCoolzic commented Dec 14, 2023

I will take a look at that later. Are you running under esp32 or esp8266?

Wont show up with esp8266 because ifdef

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants