search

ginkage / MHI-AC-Ctrl-ESPHome

ESPHome integration for MHI-AC-Ctrl project
MIT License
101 stars 35 forks source link

make framesize configurable #71

Closed RobertJansen1 closed 8 months ago

RobertJansen1 commented 8 months ago

closes #69 #68

RobertJansen1 commented 8 months ago

and introduced a version number

Dennis-Q commented 8 months ago

I have reviewed the code, and I think all is clear and should work as suggested. When I have some time (probably next week), I'll give the code a try.

I do have one remark regarding this text: "# Comment if you encounter mhi_ac_ctrl_core.loop error: -2 errors and uncomment the legacy_framesize file"

Even with the 'legacy' framesize (20) and even with the previous versions which didn't set the frame size explicitly, I still had these errors every now and then, while I don't notice any real world problems with that.. But I wasn't flooded with them multiple times a second which is the case wiith the framesize set to 33. So maybe for clarification, you might want to add 'flood' or 'flooding' to the text to clarify when to use the legacy version.

I was thinking.. Isn't it as easy to just have 2 complete (containing all components) yml example files with a similar comment as you have now saying they should try the legacy version of the yml in case they get the loop error: -2 flooding. Although your solution is really nice already, it doesn't allow copy/pasting the code directly into ESPHome (but that wasn't the case anyway since you still need to download the cpp and .h files anyway)... This is not a comment, just an open suggestion/idea. :)

Thank you for taking the time and working on the suggestions provided! Really appreciated!

RobertJansen1 commented 8 months ago

I have reviewed the code, and I think all is clear and should work as suggested. When I have some time (probably next week), I'll give the code a try.

I do have one remark regarding this text: "# Comment if you encounter mhi_ac_ctrl_core.loop error: -2 errors and uncomment the legacy_framesize file"

Even with the 'legacy' framesize (20) and even with the previous versions which didn't set the frame size explicitly, I still had these errors every now and then, while I don't notice any real world problems with that.. But I wasn't flooded with them multiple times a second which is the case wiith the framesize set to 33. So maybe for clarification, you might want to add 'flood' or 'flooding' to the text to clarify when to use the legacy version.

I was thinking.. Isn't it as easy to just have 2 complete (containing all components) yml example files with a similar comment as you have now saying they should try the legacy version of the yml in case they get the loop error: -2 flooding. Although your solution is really nice already, it doesn't allow copy/pasting the code directly into ESPHome (but that wasn't the case anyway since you still need to download the cpp and .h files anyway)... This is not a comment, just an open suggestion/idea. :)

Thank you for taking the time and working on the suggestions provided! Really appreciated!

changed the readme a bit to flooding, i don't really want to keep maintaining 2 versions of the same file was my initial thought. i will probably make it more smooth when i have the time for it, but for now i wanted to fix breaking the older units fast, as i saw it got some traction (both here and on the tweakers topic)