-
Notifications
You must be signed in to change notification settings - Fork 46
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
Package downloader #51
base: master
Are you sure you want to change the base?
Conversation
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.
.
openomni/bin/omni_listen_rfcat
Outdated
# Default variables for filename | ||
dateRecording = datetime.today().isoformat() | ||
userName = getpass.getuser() | ||
fileName = '%s-%s' % (dateRecording, userName) |
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.
I am also looking for a better way of naming the files. Now there only date and username, so finding your results back is a bit cumbersome. The free text input would be a bit to unorganised as a filename I think. Do you have any suggestions?
openomni/bin/omni_listen_rfcat
Outdated
@@ -45,7 +45,8 @@ def main(options=None): | |||
with open('../../results/' + fileName + '.json', 'a') as outFile: | |||
"""Create 1 dict for each message loop.""" | |||
package ={} | |||
messages = [] | |||
messages = {"string": [], |
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.
I've grouped the raw's all together and the strings, I first had it grouped for each step, but that was not really easy to read, see added example with the grouping I find is really helpful. @ps2 what do you think?
@@ -31,7 +31,7 @@ You can install openomni in editable mode like this: | |||
``` | |||
git clone https://github.com/openaps/openomni.git | |||
cd openomni | |||
pip install -e . | |||
pip install -e . --process-dependency-links |
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.
Is this necessary for rfcat? Reading about the security implications and how they want to deprecate this feature, I think maybe it should be avoided. We can list installation of rfcat as a separate step.
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.
For RFcat this is necessary if you want to install it with setup.py because it not registered on PiPy.
I read the option has been put back in because there is currently no other solution in the works:
pypa/pip#4187
I would like to keep it in for now because it simplifies the install steps, but I will create PR tonight on RFCat with the needed changes to add rfcat to the PiPy registry. It's quite easy to add packages to pipy yourself.
The separate step was a bit confusing for me at the time to understand what RFCat is used for. I used the RFCat first as a CC compiler for the TI stick and then also for openomni as a python package. So when I found out it can be installed as a package, I liked this, because it helps in the logic of all the different steps and have a 1 install command (I started using Docker at work so I am really into 1 line installers now ;-))
openomni/bin/omni_listen_rfcat
Outdated
sys.stdin.read(1) | ||
|
||
# Default variables for filename | ||
dateRecording = datetime.today().isoformat() |
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.
Please use python recommended naming for variables: underscores instead of camel case.
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.
done!
openomni/bin/omni_listen_rfcat
Outdated
|
||
|
||
#actionDescription = raw_input("Write what you are doing: ") | ||
with open('../../results/' + fileName + '.json', 'a') as outFile: |
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.
This fails for me. I usually work out of the top level directory, and run things like ./openomni/bin/omni_listen_rfcat
. File output should be an option; one is not always wanting to store things to disk.
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.
Good call. I've added a path argument to tackle this, plus added it to the readme. I will add the option to skip writing to file.
Added option to the command line to write what you have done into a json file after closing the omni_rfcat_read with ctrl+c. The file contains all the raw and string messages you've been receiving, to use it more easily with copy pasting in the wiki.