Conversation
|
Thanks, Is it possible to add a readme with some documentation and link it into the docs here: https://github.com/openenergymonitor/emonhub/blob/master/docs/emonhub-interfacers.md#list-of-interfacers---links-to-github |
|
Hello, Yes, sure. Laurent |
|
Hello guys, This pull request is open for more then one year ! Thanks, |
alexandrecuer
left a comment
alexandrecuer
left a comment
There was a problem hiding this comment.
I dont have a KNX gateway to test :-) but very nice work first of all 👍
| self.loop = asyncio.get_event_loop() | ||
|
|
||
| task = self.loop.create_task(self.initKnx(gateway_ip, local_ip)) | ||
| self.loop.run_until_complete(task) |
There was a problem hiding this comment.
Just a throught, is it threadsafe to use run_until_complete like that ?
There was a problem hiding this comment.
No more actual as I move the code to be compatible with recent python version, and rewrite this
| self.ser = False | ||
|
|
||
|
|
||
| def action(self): |
There was a problem hiding this comment.
Not sure this is really needed.
There was a problem hiding this comment.
no, we can remove it of course. fixed
| self.buffer.storeItem(f) | ||
|
|
||
|
|
||
| async def waitSensor(self): |
There was a problem hiding this comment.
Is this used somewhere ?
| def set(self, **kwargs): | ||
| for key, setting in self._KNX_settings.items(): | ||
| # Decide which setting value to use | ||
| if key in kwargs: |
There was a problem hiding this comment.
You could do setting = kwargs.get(key, self._KNX_settings[key]) and remove the whole if else
| metters = self._settings['meters'] | ||
|
|
||
| self.sensor={} | ||
| for metter in metters: |
There was a problem hiding this comment.
possible to use items() like you do below ?
| for metter in metters: | ||
| dpPoint = metters[metter] | ||
|
|
||
| for dpKey in dpPoint: |
There was a problem hiding this comment.
same I think you can iterate with items()
use var name like dp_config (snake case) and not dpConfig (camel case). the OEM maintainers are not using pylint to analyse their code but I think they should :-) I think the xknx lib is following this pattern and I think emonhub began like that, see the emonhub_interfacer base code
| import threading | ||
|
|
||
| from emonhub_interfacer import EmonHubInterfacer | ||
| from xknx import XKNX |
There was a problem hiding this comment.
xknx should be integrated in the dependencies....or maybe just a mention in the interfacer README
There was a problem hiding this comment.
i've mentioned it in the readme.md and the emonhub-interfacers.md
|
|
||
|
|
||
| class Updater(threading.Thread): | ||
| def __init__(self, knxIntf): |
There was a problem hiding this comment.
you dont give any var when you initialize the updater in self.start. What is knxIntf ?
There was a problem hiding this comment.
Updater is not need anymore, I've removed it
lo92fr
force-pushed
the
KNXInterfacer
branch
from
e7587f8 to
e09b386
Compare
Hello @alexandrecuer, Sorry, I've completely forget this pull request. |
Signed-off-by: Laurent ARNAL <laurent@clae.net>
lo92fr
force-pushed
the
KNXInterfacer
branch
from
c5a567f to
fca4984
Compare
3 participants
Add a new interfacer to connect to a KNX bus to collect datagram from KNX group.