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

TODOs and improvement suggestions #7

Open
eriknn opened this issue Jan 19, 2025 · 3 comments
Open

TODOs and improvement suggestions #7

eriknn opened this issue Jan 19, 2025 · 3 comments

Comments

@eriknn
Copy link
Owner

eriknn commented Jan 19, 2025

I'm not quite sure where to go further with this integration, but I have some thoughts:

  1. This integration could potentially support a lot of devices. Should all these be included in the integration? Or should users manually put them in their devices directory? If so, an update of the integration shouldn't remove devices in this directory. Or should the device "configuration" browse an online directory, where drivers could be downloaded from?
  2. Is the fact that devices are added with a .py-file acceptable, or should they be yaml files instead? Or maybe a combination? Is dynamically loading .py-files a potential security issue? This way of loading devices does give users a lot of flexibility compared to simply loading a yaml file, specially considering the ability to run code before/after fetching data.
  3. RTU support is theortically added, but I have no confirmation if this works, so that would be much appreciated.
  4. Manufacturer / device selection should ideally be two seperate dropdowns, but it's hard to populate one dropdown based on another one in HA Config Flow. Any suggestions on how to improve on this would be awesome.
  5. I think the way modbus-addresses now are grouped is a bit messy, but it needs to be done some way or another to optimize the communcation. Could this be improved somehow? How does this compare to the default modbus-integration?
  6. The integration now has a default group for "configuration variables", which shows these in a dropdown. I think ideally these variables should be on a "configuration"-page, but that requires UI-programming++
    I guess I more people start to use this integration, then these points needs to be addressed. Let me know what you think :)
@bkp1337
Copy link

bkp1337 commented Feb 1, 2025

I'd love to finally have a standard for adding modbus support to home assistant for different devices. For instance, most modbus-based heatpumps, ftx/ventilation systems etc should use mostly the same approach to their units (in temperatures, out temperatures, brine in/out, etc). If the standard would be set it would be so much easier to just add things to HA instead of having to write something up each time.
For instance, I need to add support for two Swegon Gold ventilation units. They're a bit different than to casa, but very well-documented (https://www.swegon.com/siteassets/_products-documents-archive/air-handling-units/gold-version-e-2013-2017/communication/_en/gold_e_modbus.pdf), instead of writing a plugin or a huge modbus list, i'd love if it could just be added to a general modbus controller...

@spjakob
Copy link

spjakob commented Feb 2, 2025

Some thoughts:

  • I think it's nice to have as many (tested) integrations included as possible with this integration.

  • Custom/local integrations could be placed in a separate (defined) directory that is only created by this integration, but otherwise not touched. Would be nice to somehow handle the scenario if you edit/change an existing integration and put it in your local directory also...Maybe you "must" change name, otherwise you will have identical stuff in your "dropdown" later.

  • I like the idea of an online directory, but maybe even more flexible to be able to add a custom url that will then download to your local directory? This way, it's possible to just get some URL/integration from an post in a forum and doesn't require updates to the "central directory". Off course this add just little "value" compared to download manually via local shell/ssh access etc. but still a nice feature to have.

Good question and I'm maybe not the right person to comment on this. However, in the case of "just downloading something that is posted on some forum" it may be a risk if the actual code could do "anything". In that case pure code may not be the best, unless you actually can do little review yourself or at least be able to trust that someone else have done it.
3.
I can not help here (now)
4.
I can only agree that this would be nice, but not provide any help.
5&6.... Can not provide much help here now...

Just a general question; have in any way been in contact with "mainline/official" HA? I really think this should be included (and maybe eventually replace the existing modbus integration).

@eriknn
Copy link
Owner Author

eriknn commented Feb 2, 2025

I'd love to finally have a standard for adding modbus support to home assistant for different devices. For instance, most modbus-based heatpumps, ftx/ventilation systems etc should use mostly the same approach to their units (in temperatures, out temperatures, brine in/out, etc). If the standard would be set it would be so much easier to just add things to HA instead of having to write something up each time. For instance, I need to add support for two Swegon Gold ventilation units. They're a bit different than to casa, but very well-documented (https://www.swegon.com/siteassets/_products-documents-archive/air-handling-units/gold-version-e-2013-2017/communication/_en/gold_e_modbus.pdf), instead of writing a plugin or a huge modbus list, i'd love if it could just be added to a general modbus controller...

Based on the Casa-drivers, this should be easy to implement. But I don’t understand how you propose to do this without any kind of modbus-list / plugin? All different devices typically use different addresses, scaling etc…

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

No branches or pull requests

3 participants