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

Support for Tilt Hydrometer 1 #261

Merged
merged 35 commits into from
Mar 14, 2021
Merged

Support for Tilt Hydrometer 1 #261

merged 35 commits into from
Mar 14, 2021

Conversation

cgalpin
Copy link
Contributor

@cgalpin cgalpin commented Mar 6, 2021

This pull request is a complete implementation of Tilt support using https://github.com/atlefren/pytilt to provide the data and can be used as-is. I wanted to get this merge request in for review and to make sure there were no concerns/issues. I do not have a newer Tilt, so not sure what differences there might be for those.

I would like to discuss options for next steps to further integrate this into picobrew_pico, since right now you have to setup pytilt yourself, as well as the raspberry pi image doesn't support bluetooth right now, and would need it regardless of the approach taken to further integrate it into the image.

So options, as I see it are

  1. Add bluetooth support to the raspberry pi image (needed either way). I am not sure exactly what's different about this image from a standard raspberry pi image, but just installing the needed rpms did not work for me. I will research further.
  2. Include pytilt in the image, configured to use http://localhost/API/tilt for the url, setup as a service
  3. Include the bluetooth code as part of the server - I do not know if nginx supports running python code as a service/daemon, but if not, this won't work, because of the overhead of the bluetooth setup it needs to run as a daemon

Thoughts?

This is related to #165

@chiefwigms
Copy link
Owner

Regarding bluetooth, I explicitly disabled it - it's a one line change in the image script, so don't go looking to far 🤣

@cgalpin cgalpin mentioned this pull request Mar 6, 2021
@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 6, 2021

Ha. Good thing I did not spend too much time on it. I did fire up another pi with a basic image to run pytilt on for my initial use/testing once i saw bluetooth wasn't working and then just used it, but should have looked closer before.

Ok, let me know what you want to do, but it should be easy to include pytilt in the image then, running as a daemon. I had to make no modifications and all it needs is an environment variable PYTILT_URL set (or set in pytilt.service if setting up as a service. There is another variable PYTILT_KEY which is sent as a http header X-PYTILT-KEY but I don't think it's needed.

@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 6, 2021

Oh I forgot to add these screenshots

Live:

tilt_live

History:
tilt_history

@tmack8001
Copy link
Collaborator

  1. Include the bluetooth code as part of the server - I do not know if nginx supports running python code as a service/daemon, but if not, this won't work, because of the overhead of the bluetooth setup it needs to run as a daemon

Nginx is simply a reverse proxy in front of the webserver. Isn't needed for the bluetooth code for receiving the Tilt data. I'd say best approach would be to setup pytilt in the image and potentially either always running it as a separate daemon directly (wasteful for those without tilts) or only run when a tilt_monitoring: True is specified in the server config.yaml (along with a toggle somewhere in the devices or setup/config UI).

Regardless we will need to enable bluetooth (either for everyone, or only when toggled on).

@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 6, 2021

Haha, i should have realized that since for development i am not running nginx. lol.

Yes I like the idea of driving it off of config.yaml and a way to enable/disable it from the UI so it's only running when desired. I'll do that.

I'll also see if I can integrate this into the webserver directly though - there is no need to have it running as a separate daemon i think, and if I have to modify it to enable/disable using our config.yaml, we lose the benefit of using it unchanged. managing it will be easier in process than as a separate one.

I don't see any harm in enabling bluetooth for all.

@tmack8001
Copy link
Collaborator

Sounds good to me @cgalpin bundled into the Picobrew server code or as a separate process is fine by me. The benefit of direct referencing within code is then it is built in for those not running on a prebuilt RaspberryPi image (assuming they have bluetooth enabled device).

Don't think we need a config setting for it unless we want to have a way to toggle off bluetooth as is the case now.

app/static/js/index.js Outdated Show resolved Hide resolved
@chiefwigms
Copy link
Owner

I'm fine with enabling bluetooth by default in the image - originally i was just trying to minimize any added RF

@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 10, 2021

I'm fine with enabling bluetooth by default in the image - originally i was just trying to minimize any added RF

I still like the idea of being able to disable it completely if for some reason you want to. I have not added the check yet, but I will.

I have another issue that concerns me - at least on my mac, i wasn't able to install pyBluez properly. It's failing on some dependency on a module called _lightblue which fails to build when i install pyBluez. If it's just me, then no biggie, but I don't want to break picobrew_pico on macs! I'll commit what I have for now, but if anyone with a mac wants to try install pyBluez and see if it installs cleanly, please do, and let me know what you get

sudo python3 -m pip install pyBluez

@tmack8001
Copy link
Collaborator

@cgalpin seems the github action type can't install from source the pybluez dependency. This is blocking the CI of this PR. The packaging of pybluez is weird to me since it has to be built by source, but guess that is due to the different transitive dependencies depending on the platform you are installing from.

We will likely need to add additional dependencies to the github action environment and then likely similarly will need to be done for building the raspberrypi image.

@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 12, 2021

Ok, please don't bother for now then. Once I get bleak working, which i'm guessing will have the same issue, we can address then.

@tmack8001
Copy link
Collaborator

Ok, are you close or need a hand getting the encoding of the protocol. BTW emailing baronbrew has in the past been super easy to get information on the engineering protocol of the Tilt though my asks there were more on the API protocol of the tiltpi interface and related apps than the raw Bluetooth signal encoding. Though I don't see that as proprietary and they would share it anyways.

@tmack8001
Copy link
Collaborator

Believe from what I've looked at in the past the data in the beacon broadcast is to be an array of CVS data points.

MAC,SG,TEMP
MAC,SG,TEMP

SG would need to divide by 1000

For any ABV calculations we can use this:

abv = (original_gravity - specific_gravity) * 131.25;

@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 12, 2021

No, I'm good (although packing it in for today).

It's an iBeacon so the format is published - it's not csv. I'm just slow and not used to python doesn't help :)

I've figured out the decoding for the uuid, temp, and gravity and the signal strength is there too so I may add that. But I need to turn my test into workable code and test. I should just wrap it up this evening, but my wife doesn't share this as an interest :)

If not tomorrow, I'll definitely wrap this up over the weekend.

fix small error in 'registering' a new device type (one that isn't already present in the config.yaml)
Comment on lines 346 to 348
if voltage:
graph_data.update({'subtitle': {'text': 'Voltage: ' + voltage}})
return graph_data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does tilt broadcast the voltage? I don't think so. We should change this and instead for the subtitle of the graph show the color of the device (eg. BLACK, BLUE, PINK, GREEN, etc) where in Picoferm we display voltage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not see this. I chose to show the RSSI here since i knew voltage was not supported.

For the color, if you think it's important, I was thinking we could out it in parens next to the name if it has not beed added

F0B45560-D29D-4717-B89A-2E6F9B9EA010 (Black)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i looked at this but it will need some plumbing. i think it can come later if needed. I'd rather put the effort into making it easier to "add" new devices than it is now.

app/static/js/tilt_graph_socketio.js Outdated Show resolved Hide resolved
app/static/support/tilt/tilt.md Outdated Show resolved Hide resolved
@cgalpin
Copy link
Contributor Author

cgalpin commented Mar 13, 2021

Documentation updated. The only outstanding issues I am aware of are

  1. fix the github actions to include bluez support (@tmack8001 is looking into this)
  2. test building a raspberry pi image with this supported
  3. how to enable bluetooth support for existing installs

#1 is a show stopper, but I don't think #2 and #3 are.

#2 i can look into. I see the setup is documented in scripts/pi/_readme.txt, but I am not sure what to do after that :) I'll research.

#3 @tmack8001 i think you mentioned you wanted to look into #3, but if not, let me know. It shouldn't be too hard to run system commands to do this, but for the short term, we can document the steps for power users/early adopters to enable bluetooth.

I think the restart_server function in routes_server.py can be updated to check for the existence of an upgrade shell script which can be fairly sophisticated. That script could do all the checks necessary to make sure it's running on a pi, then check for bluetooth support, enable in config.yaml, etc. I think we could even force an OS reboot there too to fully enable bluetooth automatically. Again, let me know if you want me to do this.

@tmack8001
Copy link
Collaborator

@cgalpin I fixed the bluez issue, cgalpin#3 so once that gets in I'm good with a merge. We can figure out the "no touch" upgrade path later.

@chiefwigms
Copy link
Owner

i'm good with this - @tmack8001 merge whenever you're good

@tmack8001 tmack8001 merged commit a2cfa03 into chiefwigms:master Mar 14, 2021
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.

4 participants