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.

Sign up for GitHub

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

Jump to bottom

Sequential scan #112

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Sequential scan #112

merged 7 commits into from
Jun 27, 2024

Conversation

sidlgor
Copy link
Contributor

@sidlgor sidlgor commented Jun 27, 2024

about: when testing a bit more i found this one:
ERROR:velbus-module:channel 96 does not exist for module @ address <None type:82 address:20 loaded:True

I gues this is a spontaneous temperature update when the module (submodule) is not yet completely loaded. This is possible during the scan procedure depending on timing but should never happen again after scan completion. I just changed the log to info instead of error. The error does not occur in my setup so it is hard to further investigate ... to be watched.

cereal2nd
cereal2nd reviewed Jun 27, 2024
View reviewed changes
.vs/VSWorkspaceState.json Outdated Show resolved Hide resolved
.vs/slnx.sqlite Outdated Show resolved Hide resolved
.vs/velbus-aio/FileContentIndex/92d3596d-70a2-4594-947c-a4111c3be043.vsidx Outdated Show resolved Hide resolved
.vs/velbus-aio/v17/.wsuo Outdated Show resolved Hide resolved
.vs/velbus-aio/v17/DocumentLayout.json Outdated Show resolved Hide resolved
@cereal2nd
Copy link
Owner

can you run pre-commit on this one?

then the ci should pass

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024 via email

@cereal2nd
Copy link
Owner

if you give me push access to your fork i'll do it

cereal2nd
cereal2nd reviewed Jun 27, 2024
View reviewed changes
velbusaio/controller.py Outdated Show resolved Hide resolved
@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024 via email

@cereal2nd
Copy link
Owner

this all starts to look good, i want to investigate one more thing:

the module 75 and 201 is always reloaded after a cached restart, once this is fixed i'll merge it.

INFO:velbus:Found module: type:57 address:254
INFO:velbus-module:Load Module
DEBUG:velbus-packet:Scan complete
WARNING:velbus:Waiting for module 75
WARNING:velbus:Waiting for module 201
INFO:velbus:Not all modules loaded yet, waiting 15 seconds

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024 via email

@cereal2nd
Copy link
Owner

i just debugged this and it has to do with the protocol.json for that module type

@cereal2nd cereal2nd merged commit ac557be into cereal2nd:master Jun 27, 2024
8 checks passed
@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024

Great news! Next steps are publication on pyPI and PIP INSTALL or can I already install from master (for use in has) ?

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024 via email

@cereal2nd
Copy link
Owner

do we want to enable the usecache.yes file by default?

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 27, 2024 via email

@niobos
Copy link
Collaborator

niobos commented Jun 28, 2024

As a user of Home Assistant, I would expect the Velbus integration to "just work". So needing to change a parameter when I add a module would not be my preferred strategy.

What may be feasible to implement, is a kind of auto-discovery: When there is no module in cache (or no cache), we de a full scan. But once we have a cache, we don't scan at startup. Instead, we listen for messages from/to addresses that are unknown to us. For each address that we encounter, we initiate a targeted scan to learn about the module at that address.

That way we can have a fast startup with the cached information, and still discover newly added modules by simply seeing them on the bus.

To "forget" old modules, we could do a quick "ping" on startup to see if the modules still respond to an RTR request.

@cereal2nd
Copy link
Owner

i just found a bigger problem:

  • after the intial scan it seems the bus is not read anymore, so the system stall

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 28, 2024

Some thoughts about "just works" auto discovery.
At first this might be hard to implement: I refer to the link https://forum.velbus.eu/t/velbus-home-assistant-missing-module-instances/17402/3 where cereal notes "sugestion 3 is hard to implement, as we wait until the loading is complete to advertise the entities to hass"
At second thought: do we really want that. Do we really want our configuration to change on the fly when a device get's on or offline. For example I have an installation with different segments with separate power supplies. Do I want those devices auto removed when a segment is temporarely down (anyway I am glad velbuslink does not do that)
To my preference, I like to be in control. I know when my installation is stable or has been modified and I like it to choose that moment to scan and verify.

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 28, 2024 via email

@cereal2nd
Copy link
Owner

cereal2nd commented Jun 28, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 28, 2024

ok, I have to setup my dev env again, just deleted about everything of my test fork :-(

@sidlgor
Copy link
Contributor Author

sidlgor commented Jun 28, 2024

Again just struggling to setup my dev environment (even reverted to full restore of PI backup but also that didn't work any more because my git fork is deleted)

I reinstalled from git with commands below

sudo git init
sudo git clone https://github.com/Cereal2nd/velbus-aio
sudo python -m venv /hass/velbus-aio
source /hass/velbus-aio/bin/activate
sudo chmod -R a+rw velbus-aio

Then when I try to debug an example (fe load_modules) I get a ModuleNotFoundError from the moment I call Velbus code outside the example (seems like path is not found)
image

Don't know what I am doing wrong. I got this working with the fork but it seems stupid to create a new fork. Please help me setup dev environment the right way.

@cereal2nd
Copy link
Owner

add a
pip install -e . after the activation of the venv and before you run the example.

On the use-cache file i think we should do the following:

  • enable this by default
  • make a param to the scan function to disable this
  • if we trigger a scan from hass disable the usecache

this give us the best of 2 worlds:

  • fast startup
  • once a user modifies his bus he should run a scan from the hass GUI

@cereal2nd
Copy link
Owner

cereal2nd commented Jul 2, 2024 via email

@cereal2nd
Copy link
Owner

Sorry, but I don's understand what you mean. Where should I find that velbus/manifest.json file.

Do I have to install the velbus integration that comes standard with HAS first? (I did that, but as expected that's an older version). Tried to locate velbus manifest.json but did not find anything refering velbus. I guess this is because it is an integrated and not a custom integration ? Finally removed standard velbus integration

Deactivated and removed previous debug version of velbus-aio and installed the latest version (2024.7.0) from sources sudo git init sudo git clone https://github.com/Cereal2nd/velbus-aio sudo python -m venv /hass/velbus-aio source /hass/velbus-aio/bin/activate sudo chmod -R a+rw velbus-aio cd velbus-aio pip install -e .

Package install seems ok but I can't debug ! Successfully installed velbus-aio-2024.7.0 (velbus-aio) lgor@has:/hass/velbus-aio/velbus-aio $ pip list Package Version Editable project location

backoff 2.2.1 pip 23.0.1 pyserial 3.5 pyserial-asyncio-fast 0.12 setuptools 66.1.1 velbus-aio 2024.7.0 /hass/velbus-aio/velbus-aio (velbus-aio) lgor@has:/hass/velbus-aio/velbus-aio $ ^C

image

Even if I could debug, HAS will still not load velbus-aio. Should I install velbus somewhere under custom_components? How to? Where comes pip install velbus-aio from pypi comes in? (By the way) I can only execute pip commands when python virtual env is created.

I am hitting exactly the same problems then when I tried to load my fork into HAS (before moving to master). This is definately because I miss background on setting up this environment. Please help!

aiofile is a new dependancy ,,,,

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Do I have to solve new dependency to aiofile manually. How to? What is it?

@cereal2nd
Copy link
Owner

cereal2nd commented Jul 2, 2024 via email •
edited
Loading

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Confused: I am not running debug code in a container. I just run HAS and some other integrations inside a container. Are this not two seperate problems: 1e solving dependency to aiofile (async file io ?), 2e make HAS (docker) load velbus-aio
1e: do I need to solve the new dependy manual? How to? I didn'have to solve other dependencies manually (serial)
2e: from prior dialog with niobis (running HAS docker too) I understood I have to copy the files from https://github.com/home-assistant/core/tree/dev/homeassistant/components/velbus to custom_components/velbus. This seems to be consistent with changes that have to be made to the manifest.json to reference the correct velbus-aio packet version.

So I am still stuck with two problems:

One last question just for my info: newest packet version looks installed on pypi but is nowere used (pip install velbus-aio from pypi). Where does pypi fits in ??

@cereal2nd
Copy link
Owner

cereal2nd commented Jul 2, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Finally I got it working just by modifying /usr/src/homeassistant/homeassistant/components/velbus/manifest.json within the container.
Still there seems to go something wrong (docker version ?). Velbus always does a full scan. I cannot locate the cache files. They are not created at /home//.velbuscache which could be logical but I also cannot locate them in the container. Also some module names are not read (default module type names).
Any thoughts ? Everything runs fine when debugging read_bus which of course does not happen in a container

@cereal2nd
Copy link
Owner

cereal2nd commented Jul 2, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Velbuscache is there but folder name ornamented with some guid? ... but the usecache.yes file ain't there

has:/config/.storage/velbuscache-fd4bfbbd7f96ebdced5e63ebf8f93494# ls
1.json 106.json 114.json 121.json 13.json 134.json 139.json 163.json 170.json 201.json 206.json 5.json 54.json 71.json
10.json 107.json 116.json 124.json 130.json 135.json 14.json 164.json 18.json 202.json 207.json 50.json 56.json 78.json
100.json 108.json 118.json 125.json 131.json 136.json 15.json 168.json 2.json 203.json 208.json 51.json 6.json 8.json
104.json 11.json 12.json 126.json 132.json 137.json 16.json 169.json 20.json 204.json 3.json 52.json 7.json 9.json
105.json 112.json 120.json 127.json 133.json 138.json 162.json 17.json 200.json 205.json 4.json 53.json 70.json
has:/config/.storage/velbuscache-fd4bfbbd7f96ebdced5e63ebf8f93494#

@cereal2nd
Copy link
Owner

cereal2nd commented Jul 2, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Missed that in latest implementation you no longer use usecache.yes to detect a valid cache (I liked the idea to create the file at the end of the scan because that's the point we are really sure a full scan has run to completion with success).
Anyway in read_bus cache detection is ok but in HAS docker the detection of the cache seems to fail. You can see from log below (made in parallel with velbuslink) that HAS is always doing a full scan.

image

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024

Just some more thinking. Validating the cache at the end of a successful scan seems important. Because a first full scan takes a long time the risk is considerable the user interrupts the scan. This will leave a partial (and possible invalid) cache which will be detected by next start. As a plus, implementation is a lot simpler and does not need asynchronous file scan :-)
The detection of at least 1 module can be an additional condition for marking a succesful scan

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 2, 2024 via email

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 3, 2024

Do you have already an idea why the cache files are not detected in HAS docker ?
What do you think about my proposol ? It's an easy implementation, it's robust for restarts, and some kind of transactional. It's already tested with read_bus but I can't load it in HAS docker (I think I finally found out the role of pypi, HAS auto loads the package version from there, so I would need to publish a new version. Is this correct ?)

Anyway, my configuration is now loading fine (al be it always with a full scan).
I can finally start using velbus in HAS :-)

@cereal2nd
Copy link
Owner

we can not simply get to this code, at least we need to be able to force a scan even when this file is there.
There is a methode in hass to force a scan, triggerd by the user, and this still needs to work.

i'll try to look at it today, but can't promise

@cereal2nd
Copy link
Owner

try this one:
https://github.com/Cereal2nd/velbus-aio/releases/tag/2024.7.1

its a stupid mistake, the get_cache_dir should only be used to set a default path, its not correct inside hass

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 3, 2024 via email

@cereal2nd
Copy link
Owner

home-assistant/core#121156

it will be in 2024.7.1

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 7, 2024

Hi, everything still works fine, but I have a question about the names passed from the velbus scan to HAS.
I find myself editing the cache files manually for

  • assigning names to older modules that don't have a name in their configuration (fe VMB8PB)
  • assigning channel names for pre defined channels which names cannot be edited (SelectedProgram, Pump, Temperature ...)

For the first item I guess there is not much that can be done (the module name is only known in velbuslink and simply can't be queried runtime). Assigning a default module name [moduletype]_[moduleaddress] might help a bit ...

For the second item however this causes multiple duplicate names in HAS (see picture below of my conf). This is solved by HAS by appending an instance number to the duplicates. This makes it not only hard to find the proper instance, it's id is not really maintenance friendly (adding or removing a module can cause complete mixup of entity id's).

Now my question: if the entity id could be set during the scan to something like velbus[moduleaddress]_[channelnr] this problem would be solved. I don't know if this can be done and if it is hard to do? It might also break existing configs!

For now: editing the cache files manually also works fine and elegant. However somewhere it does not feel completely right. The cache files rather become auto created config files ... on the other side: with the manual edit I can choose a friendly unique name which becomes also the entity id.

Maybe there is another more elegant solution ?

image

@cereal2nd
Copy link
Owner

for item 2 this can be solved in hass already, you can modify an entities name

so this should be sufficient

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 8, 2024

I know you can change the name, but not the entity id (which makes sense because this is really the key). Having a key that's deterministic and independant of velbus names would make it less vulnereable for config changes (identical to velbuslink where the name does not matter).
Not really a big problem, just a suggestion. I am perfectly happy with manual editing the cache files. I don't have the intension to ever clear the cache completely.

@cereal2nd
Copy link
Owner

yes thats one of the decisions i mainly regret when developing this.
I should have set the entity id to something like module_channel and pass the name as an attribute.

Now its really difficult to change, this will break almost all installations ...

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 8, 2024

"this will break almost all installations" that's what I was afraid off. The only way to solve this properly would be to introduce a compatibily mode as an installation parameter to support existing installations.
As I said this is not such a big deal. Once you know about the vulnerability you just have to be carefull with renaming velbus channels that are also used in HAS. For the channelnames that can't be edited in velbuslink: manually editing the cache files works fine too. You just have to keep in mind that these cache files really become config files and should be treated as such (backup). I guess / hope most installations aren't that complicated that this should stay manageable.
As a suggestion for Velleman: it would be nice to have a json-like export of the configuration in velbuslink. That would make thing a lot easier !

@niobos
Copy link
Collaborator

niobos commented Jul 8, 2024

yes thats one of the decisions i mainly regret when developing this. I should have set the entity id to something like module_channel and pass the name as an attribute.

Now its really difficult to change, this will break almost all installations ...

Changing this will certainly be difficult, but not impossible. We'd have to support both "modes" of addressing, but can gradually switch to new [module]_[channel] IDs for new installs or manual upgrades.

However, there should be a clear advantage to justify this effort. Both on the developer side to implement the needed changes, and on the user side when using the new ID-system.

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 8, 2024

I don't think the effort for changing the code is very high ... the pain is in breaking existing configurations.
The win for a NEW user is obvious ... whatever you decide ...

@sidlgor sidlgor mentioned this pull request Jul 9, 2024
Closed
@KevChief
Copy link

Trying to catch up on the discussion or conversation here; if someone can put a small summary on what's the open discussion, I'm happy to jump in the brainstorming phase :)

@sidlgor
Copy link
Contributor Author

sidlgor commented Jul 11, 2024

I will try to summarize:

To understand this brainstorm it's important to know that HASS identifies an entity with an entity id and a name. The entity id serves as a key. This key must be unique and is not editable. The name is rather an attribute, allows duplicates and is editable in HASS.

Historical in velbus-aio the velbus channelname was chosen to identify the entities in HASS. Because HASS needs the unique key, a key is automatically derived from the channel name (lower case, replace spaces, ...). Because the channelname is not at all unique by default an appearance number is added to the key (fe when relay1 would be defined several times the keys would become relay1, relay1_2, relay1_3, ...)

That historical choice has some consequences

  • The name given to a channel in velbus(link) becomes extremely important, changing a channelname will break any previous reference made in HASS
  • By default velbus modules of the same type will get default names. Because the name passed to HASS is not prepended by the module name this will create lot's of multiple duplicate names (keys)
  • It is a lot of work, but the problem with the duplicate channelnames can be solved by manually assigning a unique name in velbus. Only ... special channel names like SelectedProgram, Temperature, Alarm, ... cannot be edited in velbuslink!
  • Because duplicate names are appended with an appearance number this will make the generated key dependent of the order of modules (address). As an example: when we have multiple switch modules with each a SelectedProgram and we add or remove a module somewhere in the middle of our configuration, then all subsequent SelectProgram keys will point to a wrong module

The architectural solution for all above problems is simple and recognized: just enforce the entity id to something like velbus_moduleaddress_channelnr and make the FULL channelname (with module name) an attribute but ... IT WILL BREAK ALL EXISTENT CONFIGURATIONS.

Because of the breaking change the idea of supporting two modes was introduced. (mode manually selectable in yaml)

  • compatibilty mode = default existing mode
  • keyed mode (?) = new mode with proper key assignment

To wrap up

  • The compatibility mode has the important advantage that it exists and works (nothing has to be done by developer or user !)
  • The new mode solves some vulnerabilities and obvious has important maintenance advantages. Also the tedious task of manual editing all channelnames to unique values is no longer required.
  • The new mode is definitely the preferred mode for a new user ( easier maintenance, less renaming work, comparable with velbuslink module/channel handling)

I solved some of the problems by manually editing the json cache files. This works fine but from architectural point this does not feel right. (cache files become config files in reality)

Finally this is up to the owner. If it's decided to implement the new mode I will definitely make the effort to rework my config, otherwise I can also live fine with current implementation.
Just let me know if I can help in any way ...

@KevChief
Copy link

Thanks @sidlgor for the above summary!

Very interesting brainstorm. For now, I can't think of anything clever to get out of this legacy issue described. We could try an automatic migration but that would mean understanding the current set-up and mapping of all modules to devices and entities as well as re-configuring every place in HA these are used (some kind of migration script , but not sure whether that's even possible access rights' wise by an integration inside of HA).

The idea above could be a good middle ground: those who want to keep the current set-up can do so; those who want to migrate and thus want to invest the effort of re-configuring everything necessary, can migrate manually by changing that config parameter. New set-ups always default to the new method; at some point you would hope everyone has migrated and you can deprecate and eventually delete that part of the code)

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

@cereal2nd cereal2nd cereal2nd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sidlgor @cereal2nd @niobos @KevChief