-
Notifications
You must be signed in to change notification settings - Fork 6
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
solar+ drivers #57
base: master
Are you sure you want to change the base?
solar+ drivers #57
Conversation
updating changes
get latest changes
updating code
pulling latest changes
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: why are there multiple setpoints in a message with a given timestamp?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and yes; these are both conventions
Double DemandC = 58; | ||
|
||
//epoch time | ||
uint64 time = 59; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.py
file inpython/pyxbos/pyxbos