-
Notifications
You must be signed in to change notification settings - Fork 11
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
WIP: Add Smappee Infinity #240
base: main
Are you sure you want to change the base?
Conversation
2ba0536
to
7ad0bcc
Compare
@kulti can you please check Smappee class implementation? I'm not sure that I did it well. |
display_name: Smappee Infinity | ||
description: All-in-one energy management system. | ||
icon: enapter-home | ||
vendor: igd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут smappee
должно быть?
enapter.send_telemetry({ status = 'warning', alerts = { 'no_data' } }) | ||
end | ||
else | ||
enapter.send_telemetry({ status = 'warning', alerts = { 'no_service_location_name' } }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По сути это означает, что SERVICE_LOCATION_NAME обязательный параметр, но в конфиге написано required = false
.
Мне еще нравится делать так, чтобы слать один общий алерт not_configured
, если не сконфигурено. Тогда во время отправки телеметрии всегда можно будет полагаться, что конфиг настроен. Могу показать, если интересно этот момент улучшить.
enapter.send_telemetry(telemetry) | ||
else | ||
telemetry['status'] = 'warning' | ||
enapter.send_telemetry(telemetry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь будет непонятная причина ворнинга. А если до этого были алерты, то они останутся. Особенно странно будет выглядеть алерт no_service_location_name
.
PS. Читая код дальше, понял, что тут проблема с зонами ответственности — телеметрия в разных участках кода собирается.
conn.access_token, conn.new_token, conn.expires = conn:refresh_token() | ||
end | ||
|
||
return conn, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут получилось так, что если у conn нет expires, то будет реконнект. Так и задумывалось?
if not client_secret or not client_id or not username or not password then | ||
return nil, 'not_configured' | ||
else | ||
conn = smappee.new(username, password, client_secret, client_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если конфиг поменялся, то, наверно, тоже стоит заново коннектится. Это вряд ли большая проблема, т.к. сценарий маловероятный и можно просто перезагрузить устройство, чтобы заработало с новым конфигом.
В теории может быть такой сценарий:
- Коннект есть, все работает.
- Изменился пароль, стали приходить ошибки, что не получается данные забрать. (404 или 403).
- Поменяли пароль в конфиге, но ошибки никуда не ушли.
return nil, err | ||
elseif response.code ~= 200 then | ||
enapter.log('Request returned non-OK code: ' .. response.code, 'error') | ||
return nil, response.code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ошибка — это строка, а не число. Думаю, тут вместо лога лучше эту строчку ошибкой вернуть.
self.client = http.client({ timeout = 5 }) | ||
|
||
self.access_token, self.new_token, self.expires = self:get_token() | ||
if not self.access_token or not self.refresh_token or not self.expires then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут опечатка self.refresh_token
-> self.new_token
.
end | ||
|
||
if response ~= nil then | ||
return response['access_token'], response['refresh_token'], response['expires_in'] + os.time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если response['expires_in']
будет nil, то случится паника.
Я бы предложил не возвращать все эти штуки наружу, а валидировать тут и сохранять во внутренние поля. И возвращать ошибку, если что-то не так. Так код будет чище, проще, и абстракция не потечет.
else | ||
local location_name = config.read(SERVICE_LOCATION_NAME) | ||
if tostring(location_name) ~= '' then | ||
conn:set_service_location_id(location_name, conn:get_service_locations()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь странно, что сначала берем все локейшены у объекта conn, а потом обратно их ему же отдаем. Думаю, будет лучше оставить метод set_service_location_id
с одним параметром set_service_location_id
, а он дальше сам разберется, что делать.
Также тут получилось, что на каждый запрос телеметрии запрашивается спиоск локейшеном. Я не смотрел в доке, но, думаю, вряд ли стоит это делать больше одного раза, т.к. локейшен id не поменяется.
Единственный нюанс, если переконфигурировали устройство. Тогда да, тогда неправильно будет работать. Но это я бы по другому решил, с помощью колбэков из библиотеки config. Это то, о чем я предлагал в другом дискашене про not_configured
рассказать.
end | ||
end | ||
|
||
self.inputs = inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем их складывать в поле таблицы? Можно просто вернуть, т.к. они запрашиваются каждый раз. Возможно, это тоже избыточно, тогда есть смысл один раз запросить и сохранить.
end | ||
|
||
function Smappee:get_electricity_consumption(from, to) | ||
local params = 'aggregation=1&' -- level of detalization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Обсуждали с @kulti, как избежать такого «ручного склеивания». Нашли библиотечку: https://luarocks.org/modules/golgote/net-url. Можно попробовать так:
local url = require('net.url')
function Smappee.new(...)
...
self.url = url.parse('https://app1pub.smappee.net/dev/v3')
...
end
function Smappee:get_electricity_consumption(from, to)
url = self.url / 'servicelocation' / self.service_location_id / 'consumption'
url:setQuery({ from = from, to = to })
...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Спасибо большое, попробую :)
No description provided.