MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant
GNU Lesser General Public License v3.0
114 stars 62 forks source link

Feature/common io t #12

Closed ChristopherRogers1991 closed 1 year ago

ChristopherRogers1991 commented 5 years ago

Description

Implement this skill according to the CommonIoT framework.

ChristopherRogers1991 commented 5 years ago

https://github.com/MycroftAI/mycroft-core/pull/2150 will need to be merged before this. Ideally https://github.com/MycroftAI/mycroft-skills/pull/1032 will go in before this as well.

andlo commented 4 years ago

looking to try this, but it fails badly. I did install skill-iot-control and teh CommonIoT/state is merged now.

~~~~| mycroft.skills.mycroft_skill.mycroft_skill:on_error:805 | An error occurred while processing a request in Skill Io T Control
Traceback (most recent call last):                                                                                                
  File "/home/andlo/mycroft-core/mycroft/skills/mycroft_skill/event_container.py", line 66, in wrapper                            
    handler(message)                                                                                                              
  File "/opt/mycroft/skills/skill-iot-control.mycroftai/__init__.py", line 178, in _register_words                                
    normalized = _normalize_custom_word(word)                                                                                     
  File "/opt/mycroft/skills/skill-iot-control.mycroftai/__init__.py", line 345, in _normalize_custom_word                         
    word = word.lower() 
AttributeError: 'NoneType' object has no attribute 'lower'           

Cant figure out if error is in this skill or the skill-iot-control.

ChristopherRogers1991 commented 4 years ago

On mobile at the moment, so a little limited in how much I can dig in, but from the trace and what I can remember about how this works, the home assistant skill is trying to register the names of custom scenes, actions, etc. Some how, one of the words it's passing through is a None. The control skill should probably be more resilient to that (filter out None instead of failing hard), but it's probably also worth investigating why the Home Assistant skill is sending None.

Some of the relevant code sections:

The relevant code in the home assistant skill is probably here, where it builds the entity and scene maps:

https://github.com/MycroftAI/mycroft-homeassistant/blob/feature/commonIoT/__init__.py#L166-L184

The CommonIoT skill base class then does the registering of the words here:

https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/skills/common_iot_skill.py#L443-L472

Which ultimately sends them (via the message bus) to the control skill, which registers them when adapt here:

https://github.com/MycroftAI/skill-iot-control/blob/master/__init__.py#L172-L181

andlo commented 4 years ago

Ohhh I see. This function will return an empty dict if there isnt any scenes

    def _build_scenes_map(self, entities: dict):
        results = dict()
        for id, name in entities.items():
            if not name:
                continue
            name = name.lower()
            if self._domain(id) == _SCENE:
                results[name] = id
        return results

And when registering of the words like this

 self._register_words(self.get_scenes(), SCENE)

Ill gues that goes bad..... I dont think it is HA that returns Nnone as a scene, but if he scenes dict is empty, and used to register words - then he skill tryes to register None.

So I think there is need of some checks before registering.

ChristopherRogers1991 commented 4 years ago

Hmm... I'm not sure that's correct. The get_scenes method just returns the keys of that dictionary: https://github.com/MycroftAI/mycroft-homeassistant/blob/feature/commonIoT/__init__.py#L220-L221

So if the dict is empty, that should just be an empty list.

Then, in the CommonIoTSkill base class, we check if there are items in the list here: https://github.com/MycroftAI/mycroft-core/blob/dev/mycroft/skills/common_iot_skill.py#L456.

So I would be surprised if an empty dict is causing the break.

Perhaps it would be worth adding some logging to the functions that make the scenes and entities dicts?

andlo commented 4 years ago

yes - i can also set debugger breakpoint at that place to se what is going on. But interesting thing is that I dont have scenes - but it could ofcause be som other entities that has a None value in the Dict. Ill look more at that when not at work ;)

andlo commented 4 years ago

Trying to figure out what is going on.....Now it fails with this

 16:10:37.515 | ERROR    | 17980 | mycroft.skills.skill_loader:_create_skill_instance:249 | Skill initialization failed with AttributeError("'NoneType' object has no attribute 'forward'")        Traceback (most recent call last):                                                                                                                                                                 
  File "/home/pi/mycroft-core/mycroft/skills/skill_loader.py", line 243, in _create_skill_instance                                                                                                 
    self.instance.initialize()                                                                                                                                                                     
  File "/opt/mycroft/skills/homeassistant.mycroftai/__init__.py", line 161, in initialize                                                                                                          
    self.register_entities_and_scenes()                                                                                                                                                            
  File "/home/pi/mycroft-core/mycroft/skills/common_iot_skill.py", line 474, in register_entities_and_scenes                                                                                       
    self._register_words(self.get_entities(), ENTITY)                                                                                                                                              
  File "/home/pi/mycroft-core/mycroft/skills/common_iot_skill.py", line 460, in _register_words                                                                                                    
    self.bus.emit(message.forward(_BusKeys.REGISTER,                                                                                                                                               
AttributeError: 'NoneType' object has no attribute 'forward'                                                                                                                                       
 16:10:37.559 | ERROR    | 17980 | mycroft.skills.skill_loader:_communicate_load_status:286 | Skill homeassistant.mycroftai failed to load                                                         

it seems it has gotten all the entities from HA, but failing when trying to register the words :(

ChristopherRogers1991 commented 4 years ago

This error looks like it's related to a change from @jarbasai, based on the blame: https://github.com/MycroftAI/mycroft-core/blame/dev/mycroft/skills/common_iot_skill.py#L460

I'm not sure what dig_for_message is supposed to do, but it like like it's returning a None. I don't know if the issue is a bug with that function (if it's not supposed to return a None), or if there should be a check here, and it needs to fall back to just emitting the message as it used to, rather than using message.forward.

stratus-ss commented 1 year ago

I'm closing this. We're not going to be adopting this now that the mycroft project is community run.

We can revisit in the future if there is sufficient interest