solar+ drivers#57
Conversation
updating changes
get latest changes
updating code
pulling latest changes
gtfierro
left a comment
gtfierro
left a comment
There was a problem hiding this comment.
Looking good! Thanks for submitting the changes. I requested a few small things in the files, but we should be good to merge after that
|
|
||
| message FlexstatActuationMessage { | ||
| uint64 time = 1; | ||
| repeated FlexstatSetpoints setpoints = 2; |
There was a problem hiding this comment.
Just curious: why are there multiple setpoints in a message with a given timestamp?
There was a problem hiding this comment.
So: our MPC controller publishes a list of setpoints for the next 24 hours. I haven't pushed the updated thermostat driver that does the actual controls yet, but that driver will handle this.
There was a problem hiding this comment.
In that case, the setpoint message should have a field that indicates the time (+ duration) for the setpoint. I think there is a "setpoint schedule" message already defined in the iot.proto file
There was a problem hiding this comment.
There is a message called change_time in FlexstatSetpoints that indicates the time of change for each setpoint (line #11). duration might be a good idea. Let me check with the MPC team. Thanks!
Oh! I'll take a look at the setpoint schedule though.
| Int64 fan_status = 33; | ||
| //epoch time | ||
| //unit: seconds | ||
| uint64 time = 34; |
There was a problem hiding this comment.
Why not have a single FlexStat thermostat message that wraps the timestamp? In any case, you should put the time field at the top of the message (although you do not need to rename/renumber the field)
There was a problem hiding this comment.
Do you mean a single message for state and actuation together?
And yes, I'll move the time field to the top. Is this just a convention?
There was a problem hiding this comment.
Yes and yes; these are both conventions
| Double DemandC = 58; | ||
|
|
||
| //epoch time | ||
| uint64 time = 59; |
There was a problem hiding this comment.
Time field should go at the top of the message definition; do not need to rename the field
| @@ -1,118 +0,0 @@ | |||
| from pyxbos.driver import * | |||
There was a problem hiding this comment.
Why is this file deleted? Current installations are using this driver. Please undelete or rename your other driver file to this
There was a problem hiding this comment.
My bad! Will revert this deletion.
The new driver that I added has a lot more fields including a calculation solar irradiance measurement.
There was a problem hiding this comment.
If the new driver is fully backwards compatible, then why not replace the dark_sky.py driver with the new driver but keep the same file name?
There was a problem hiding this comment.
It actually isn't. To calculate the solar irradiance, you need to install a new python library. I think it'll be better if we keep the two files separate.
Additionally, in the new driver both the predictions and the current values are part of the same class - to avoid exhausting the "free" API calls using the API key.
2 participants
files associated for 4 devices developed as part of Solar+ project:
Files:
proto/(with bindings inproto/andpython/pyxbos/pyxbos/respectively)ingester/plugins/python/pxbos/pyxbos/drivers/modbus_driver.pyfile inpython/pyxbos/pyxbos