Skip to content

Conversation

@mhkrebs
Copy link
Contributor

@mhkrebs mhkrebs commented Jun 15, 2025

Put the new switch code under USE_SWITCH in case the YAML config doesn't have a "switch:" section. Move the pin writes into the YAML config. See also issue #4.

@mhkrebs mhkrebs closed this Jun 15, 2025
@mhkrebs mhkrebs changed the title Fix compile errors TRASH: Fix compile errors Jun 15, 2025
@mhkrebs mhkrebs reopened this Jun 15, 2025
@mhkrebs mhkrebs force-pushed the fix/4-fix-compile-errors branch from 35112af to 1f43690 Compare June 15, 2025 08:04
@mhkrebs mhkrebs changed the title TRASH: Fix compile errors Fix compile errors Jun 15, 2025
@mhkrebs
Copy link
Contributor Author

mhkrebs commented Jun 15, 2025

I moved the digitalWrite() stuff into the YAML config, since I have a different board. And that output: config seemed to work for my pins, at least.

It's possible the inverted flag is backwards, though, I'm not sure. I used GPIO14 and GPIO12 based on what the pins look to be on the esp8266.

@mhkrebs mhkrebs marked this pull request as ready for review June 15, 2025 08:10
…the YAML config doesn't have a "switch:" section. Move the pin writes into the YAML config.
@mhkrebs mhkrebs force-pushed the fix/4-fix-compile-errors branch from 1f43690 to c928684 Compare June 15, 2025 08:48
break;
}

// ESP_LOGV(TAG, "Calculated checksum over %d bytes => 0x%02X", HDR_SIZE + this->recv_buffer.hdr.len, crc);
Copy link
Owner

Choose a reason for hiding this comment

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

Agree with this change, but it should not have been broken the compilation since the line is commented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I found that with an older revision where it was still uncommented. So I just transferred my fix to this line even though it's now commented out.

void send_turn_off_cmd();

protected:
void send_cmd(const uint8_t * buffer, uint8 len);
Copy link
Owner

Choose a reason for hiding this comment

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

Agreed

@htumanyan
Copy link
Owner

I've reviewed the changes. Thank you, again, for taking time to do these.

As I've reviewed, I've realized that I've missed checking in some local changes, which I did now. Would you, please, try to compile the top of the branch and let me know if it works?

Also, for the IFDEF switch - is there a reason why you ifdefed it? The idea is to always have a switch component and the derived NavienOnOff switch to manage the on and off state for the device.

Lastly, I see the move of pin controls to the navien.yml. Not to be critical, but just to understand - what does it achieve compared to the version in the code?

Thank you,
Hovhannes

@mhkrebs
Copy link
Contributor Author

mhkrebs commented Jun 16, 2025

As I've reviewed, I've realized that I've missed checking in some local changes, which I did now. Would you, please, try to compile the top of the branch and let me know if it works?

I had to make just a couple changes, but it compiles for me now. I can update this PR once I know what to change per the following...

Also, for the IFDEF switch - is there a reason why you ifdefed it? The idea is to always have a switch component and the derived NavienOnOff switch to manage the on and off state for the device.

I was thinking that was for the Hot Button, but it looks like you added a separate button for that. So is the NavienOnOff for turning the whole thing off? Does that work with just the 4 wires hooked up (12V, GND, and uart wires)? (I have to admit that I'm still trying to grok pins and how they relate to the hardware.)

And will the ESP remain powered if the Navien is turned off and the ESP is hooked up to the 12V from the board? I'm just curious since, either way, I could imagine you'd want to make it available in the device -- even if Home Assistant should never trigger it if you can't turn it back on remotely. :)

For the new NavienHotButton, is that only applicable if you have recirculating and a physical hot button? I don't have recirculating so I didn't want to put that in my .yaml config (hence the "IFDEF SWITCH" when I thought that was the Hot Button). So I put #ifdef USE_BUTTON around the new NavienHotButton. But if there's a reason to support sending that hot-button-command even if you don't have a Hot Button, then I can see why it should be in the .yaml.

Or, is your thinking that you want this ESP to continue working even if someone adds the Hot Button to the Navien later? ...i.e. so we wouldn't have to rebuild the firmware? That's not a bad idea either.

Lastly, I see the move of pin controls to the navien.yml. Not to be critical, but just to understand - what does it achieve compared to the version in the code?

Correct me if I'm wrong (it's very possible I am), but I thought that was specific to your serial hardware. I have an "esp32-c6-devkitm-1" and an RS485 breakout board for it, which I assumed would not want those same pins written in setup.

For my board, I had put the following in my .yaml to match the documentation I read for it:

output:
  - platform: gpio
    pin: GPIO2
    id: uart_enable_output

@mhkrebs
Copy link
Contributor Author

mhkrebs commented Jun 21, 2025

Also, for the IFDEF switch - is there a reason why you ifdefed it? The idea is to always have a switch component and the derived NavienOnOff switch to manage the on and off state for the device.

Thinking about it, the sensors are conditional so they don't have to all be listed in the config. Should those be required too? I.e. sensor.py could presumably give an error instead if one's missing? I can imagine either way, that everything's optional in the yaml, or that's everything's required in the yaml.

I left my #ifdefs in this PR rather than remove the conditional'ness of the sensors, but obviously you can take or leave those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants