Code review and PEP8 clean-ups

@NateSchoolfield,

Some quick take aways, generally it isn't a python-y way to use global scoped variables. It might be better to pull all of the main defs into a class block. Also did a PEP8 pass to get thing lined up.

I took a look at the project files, and I have a few questions for you regarding a couple of sections:

In process_gyro_data, it looks like you are iterating through and getting the cumulative value and a last value. I think there might be a more elegant way to do this. What does gyro_data look like? List of dict with only keys ['x','y','z']? Might be a good candidate to create a class to automate getting a delta out of.

#get the last values from a dict
for gyro_datum in gyro_data:
    gyro_delta_x = abs(gyro_datum['x'] - gyro_last_x)
    gyro_last_x = gyro_datum['x']
    gyro_delta_y = abs(gyro_datum['y'] - gyro_last_y)
    gyro_last_y = gyro_datum['y']
    gyro_delta_z = abs(gyro_datum['z'] - gyro_last_z)
    gyro_last_z = gyro_datum['z']
    gyro_delta_sum = gyro_delta_x + gyro_delta_y + gyro_delta_z
    gyro_movement += gyro_delta_sum
#set the last values to a dict

I noticed you're using the items method to loop over the keys but are throwing away the value part of the pair. If you don't need the value part of the pair, you can simplify the reference and do the following:

for data_type in sleep_data:

By default when python uses the in operator on a dict, you get a list of keys without having to declare specifically the keys method. Which can be pretty handy. I've gone ahead and cleaned these up.

In connect, once it connects once it will never try to reconnect if it looses the link. Might be better for this to be the entry point for the thread runner vs start_data_pull. After reading through the connect/start_data_pull I got the idea of how you're calling the thread start. Where you assume the connection is valid when you call the thread. I think that this might end up calling connect twice or multiple times at start up. I feel this workflow could be clarified and cleaned up a little. Nice to see the sleep on the retry, smart.

In miband.py you test for xrange. As it's been deprecated in python3, you should just use range rather than re-declaring it. Generally speaking, there are a ton of magic values for commands. For manageability might be smart to pull those into an contants.

Generally the enum classes don't seem very python-y. They work, but not sure you wouldn't just import the module and do <module>.<name> to reference the values. Generating classes for everything seems like a little more work than just importing some extra module files. Not saying one way or the other is the "correct python way™", just not how I personally would have done it.

Attached is the PR for the updates and changes. As I don't have a band to test with, it will need some testing, so you might want to clone my branch into a new directory to test it.

Cheers, --@rmackinnon

Thank you!!

@NateSchoolfield,

Some quick take aways, generally it isn't a python-y way to use global scoped variables. It might be better to pull all of the main defs into a class block. Also did a PEP8 pass to get thing lined up.

I knew there must have been a better way to do this, I'll learn about classes tonight and incorporate this.

I took a look at the project files, and I have a few questions for you regarding a couple of sections:

In process_gyro_data, it looks like you are iterating through and getting the cumulative value and a last value. I think there might be a more elegant way to do this. What does gyro_data look like? List of dict with only keys ['x','y','z']? Might be a good candidate to create a class to automate getting a delta out of.

This is correct. Here's a single return from the band (aka 'gyro_data'):

[ {'x': -256, 'y': -6, 'z': 13}, {'x': -258, 'y': -4, 'z': 12}, {'x': -258, 'y': -4, 'z': 12} ]

In connect, once it connects once it will never try to reconnect if it looses the link. Might be better for this to be the entry point for the thread runner vs start_data_pull. After reading through the connect/start_data_pull I got the idea of how you're calling the thread start. Where you assume the connection is valid when you call the thread. I think that this might end up calling connect twice or multiple times at start up. I feel this workflow could be clarified and cleaned up a little. Nice to see the sleep on the retry, smart.

The reconnect code is in the start_data_pull function because of how the BLEDisconnectError exception bubbles up from the band class when it disconnects. Handling it there seemed appropriate, but I'm a (relative) python newb so I'm open to learning better ways of doing this.

The first call to connect() runs synchronously in the main thread, so start_data_pull isn't called until after it's (successfully) connected. Putting it at the top of the start_data_pull function seems reasonable.

I can't take credit for the sleep there, it came from satcar77:miband4

In miband.py you test for xrange. As it's been deprecated in python3, you should just use range rather than re-declaring it. Generally speaking, there are a ton of magic values for commands. For manageability might be smart to pull those into an contants.

Generally the enum classes don't seem very python-y. They work, but not sure you wouldn't just import the module and do <module>.<name> to reference the values. Generating classes for everything seems like a little more work than just importing some extra module files. Not saying one way or the other is the "correct python way™", just not how I personally would have done it.

I'll remove the xrange and Queue tests as this project is solidly in python3 territory.

Most of miband.py came from the original project (satcar77:miband4) and I haven't changed much except to remove the bits that aren't obviously useful for this project (music controls, etc) and add bits that are (gyro data handling, mainly).

I don't fully understand your comment about enum classes, but this is entirely due to my newbness. As I learn about classes tonight I'm sure my understanding will develop.

As the miband4 BLE protocol is pretty much completely undocumented, there are definitely a lot of magic values flying around due to the nature of reverse engineering. I like the idea of moving them into constants.py, but I'll have to be clear about which ones I'm certain about and which ones are still mysterious.

I've been focusing on bluesleep.py and tweaking miband.py as necessary. I'll spend some time with miband.py and try to learn enough to incorporate your suggestions.

The PR looks great (TIL about PEP8), only one minor note on the changes to update_graph_data(). I'm using the convention of "data" as a plural of "datum". For context, the gyro_data list I mentioned above would be "data", while a single item from the list would be a "datum".

I've tested the modified code and after fixing a minor typo (line 257) it worked perfectly.

I'll merge the PR into a separate branch, make the minor fixes/modifications, and then merge that into master, once I figure out how to do all of that :)

Thank you for your help, I appreciate it.

BeanCounterTop / blesleep

Code review and PEP8 clean-ups #1