ECESeniorDesign / greenhouse_envmgmt

Development and prototype software for interfacing raspberry pi to I2C/SMBus sensors
BSD 2-Clause "Simplified" License
1 stars 0 forks source link

Develop more simple API for sensors and Controls #13

Closed jeffrobots closed 8 years ago

jeffrobots commented 8 years ago

Each module should have a simple way to control all of its attributes. For the sensors, this means that updating sensors should be able to be done (but not required to be done) with one or two function calls.

For controls, this means that the user should be able to turn on and off individual control units on request with one or two function calls AND that the user should be able to specify a list of units that need to be turned on.

jeffrobots commented 8 years ago

@cconklin

Could you look over the classes for the sensors and controls to see if the API is workable (or if you have suggestions on cleaning it up). I now sort of wish I had designed the code to be more modular so that I could abstract a bit more, but that would be a low priority issue for sure at this point.

For sensors, you can mostly look at update_all_sensors(), update_instance_sensors(), and save_instance_sensors(). Note that update_all_sensors and update_instance_sensors() contain an optional argument for updating the soil moisture sensors as well since we may not want to constantly power them up. Having the option to only update them on the hour - for example - will extend their lifetime significantly.

For controls, you can mostly look at control() and get_water_level().

These are really the only methods I would see you interacting with unless you want to do specific sensor updates or anything weird that I haven't thought of.

cconklin commented 8 years ago

@jeffrobots

Controls

  1. From an API standpoint, it doesn't make much sense to have get_water_level (a sensor method) to be present on a control interface. It looks like you put it here because the water tank sensor is connected to the control board.
  2. How does the bus parameter change? I noticed in the control test file that you always set bus = smbus.SMBus(1). Is this specific to the test you are running, or is this a universal constant. If the value is more of a constant (or isn't likely to vary / has a reasonable default), I would recommend making it part of the ControlCluster object (i.e. set to an instance variable during initialization) and having a default value.
  3. Question: are we using a valve? I see some code that updates some valve state.

Sensors

See point 2 above. Other than that, the API looks pretty good.

jeffrobots commented 8 years ago

1) Good point. I originally put it there in case we wanted the pump operation to ask if it can turn on by sensing water level each time. This is now obsolete, since the pump can ping the database instead. My other reason being that it will be using a separate adc that sits on the controls.

2) The bus object is static, but it's global to the entire system. I hadn't thought to hide instances within sensor and control objects, but that would definitely clean up some of the methods so long as the smbus API supports that.

3) Ehh. I don't plan on using one now since we have the mist pump. It's more or less a feature that some users would want in the future, so I saw no reason to pull it out. I'm not that attached to it though.

cconklin commented 8 years ago

Also, if a control cluster with ID 3 or 4 tries to turn off the pump, it looks like nothing will happen, since it won't send the update to the correct bank.

jeffrobots commented 8 years ago

I'm not sure I follow your last point. The pump pin and bank are pulled from the controls class and are hard-coded to bank 0 pin 0. The pump requests list is dynamically sized and accessed by instance IDs. I'll take a look at it this afternoon to double check.

cconklin commented 8 years ago

When you call update, it sends the mask for the bank to the bus. Since the pump bank is hardcoded to 0, when a control with ID 3 or 4 (bank 1) is updated, the pump will see no change. Is there something I am missing?

jeffrobots commented 8 years ago

Ahh. Good catch. That's a simple patch. I'll put it in the API update to reflect your points this afternoon.