Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

solar+ drivers #57

wants to merge 6 commits into from

Conversation

anandkp92
Copy link
Contributor

files associated for 4 devices developed as part of Solar+ project:

  1. Flexstat Bacnet thermostat
  2. Parker Refrigerator controller
  3. Dark sky weather station
  4. Wattnode power meter

Files:

  • .proto files (and corresponding go and python bindings) in proto/ (with bindings in proto/ and python/pyxbos/pyxbos/ respectively)
  • ingester plugin files in ingester/plugins/
  • .py driver files and .yaml config template files in python/pxbos/pyxbos/drivers/
  • updated modbus_driver.py file in python/pyxbos/pyxbos

Copy link
Owner

@gtfierro gtfierro left a 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;
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

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

Copy link
Contributor Author

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;
Copy link
Owner

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)

Copy link
Contributor Author

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?

Copy link
Owner

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;
Copy link
Owner

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 *
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Owner

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants