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
Improve reliability of ww pump / oref0-subg-ww-radio-parameters.sh (especially with TI USB) #357
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.
LGTM. I don't use a WW pump, so happy to merge after someone else 👍 's
I think it would be better to modify the script so that the serial device is an argument passed to I'd also suggest writing error messages to stderr. Then if the user uses a TI stick, they'd call something like |
How is this used in the loop? I can help testing but our loop doesn't use the scripts and I can't find usage documentation. |
…r TI USB users this improves openaps#357 added in oref0-setup.sh script disables subg_rfspy reset.py because it can hang a pump loop (seen with TI USB) will issue a oref0-reset-usb if the pump serial device is gone from the python script the python script will also take care of a timeout (in case the subg-ww-radio-parameters scripts is not finished within 30 seconds)
@cjo20 i removed the serial device from the shell script and now use a python script for the wrapper. This also implements the timeout for the shell script that was previous implemented in a shell script. @sulkaharo if you have a TI USB and a WW-pump you can add 'oref0-subg-ww-radio-parameters.py --resetusb` at the front of the mmtune alias. This will reset the USB subsystem in case the /dev/mmeowlink device was gone. This happened quite a lot with my TI USB stick on a RPI3. |
If you don't have your TI connected to USB you don't supply Can anybody with a TI USB and a WW pump confirm that it works for them as well. Should not change the behaviour for other rigs afaict. |
The script no longer resets the cc1110 device unless connected via USB; I can't see any code in the wrapper to do it, and it's been commented out in Also, it would be better to pass the value in as a command line option rather than setting an environment variable. You can set SERIAL_PORT in the wrapper script if you like, but then you should call |
@cjo20 : I'll include Edison and ERF radio: reset_spi and reset.py |
I think he said to try it without the reset.py. As far I can remember the explorer board and TI stick via SPI are both perfectly ok with reset.py. I'm not sure why it locks up your TI stick connected via USB. It's been a while since I used mine via USB, but I don't think it locked up my TI stick. Also: Having an issue and PR created at almost exactly the same time isn't really useful, it just splits the same discussion in to multiple places. |
@cjo20 : i noticed having a pull request and issue is not the best solution 😄 I'll reinstall a new PI3 rig from scratch to see if I can reproduce the reset.py hang on a freshly installed Pi3/TI USB/WW-pump rig. Other things / hunches I have that might cause the TI USB hang with reset.py
|
So where are we with this code? Can anyone confirm whether it's better than what's in dev now? If so, we might want to go ahead and merge it before the release to master, and then we can do future patch releases if needed to fix any major bugs we find later... |
I'm not sure that anyone has managed to run successfully with it yet. |
Does that mean that what's currently in dev is a better release candidate for now? Or is that not working yet either? Sorry, I just don't use any of this WW stuff, so I'm depending on y'all here. |
I think the changes are predominantly for the TI stick via USB and making sure that device can be found. I don't think I can see any changes that would improve WW with TI SPI or Explorer performance or reliability. |
Ok, I'm retargeting to next-dev then so we can test thoroughly without time pressure. |
Current dev code does not use pump device from pump.ini, but makes some weird assumptions, see e.g. https://github.com/openaps/oref0/blob/dev/bin/oref0-find-ti.sh#L8 I agree that this change does not improve WW pump with TI SPI or Explorer, but it does make the TI USB stick usable on my dev / mobile rig. This wasn't the case with current dev . I want to reinstall a completely fresh rig tomorrow to see if the reset.py works with that with TI USB without problems, but I think that will also fail without these changes. |
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.
Only reviewing the oref0-setup.sh part. I'll leave it to people who speak Python better and/or who use WW pumps to review the rest.
bin/oref0-setup.sh
Outdated
if [[ $TI_USB_WW =~ ^[Yy] ]]; then | ||
ti_usb_ww="yes" | ||
else | ||
ti_usb_ww0"no" |
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 looks like a syntax error?
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.
fixed
bin/oref0-setup.sh
Outdated
# Hack to check if radio_locale has been set in pump.ini. This is a temporary workaround for https://github.com/oskarpearson/mmeowlink/issues/55 | ||
# It will remove empty line at the end of pump.ini and then append radio_locale if it's not there yet | ||
grep -q radio_locale pump.ini || echo "$(< pump.ini)" > pump.ini ; echo "radio_locale=$radio_locale" >> pump.ini | ||
# Hack to check if radio_locale has been set in pump.ini. This is a temporary workaround for https://github.com/oskarpearson/mmeowli/issues/55 |
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.
Erroneous deletion?
bin/oref0-setup.sh
Outdated
@@ -169,6 +173,19 @@ if [[ -z "$DIR" || -z "$serial" ]]; then | |||
# Force uppercase, just in case the user entered ww | |||
radio_locale=${radio_locale^^} | |||
|
|||
# check if user has a TI USB stick and a WorldWide pump and want's to reset the USB subsystem during mmtune if the TI USB fails | |||
ti_usb_ww0="no" # assume you don't want it by default |
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.
What if anything does this variable do? Seems like a duplicate of ti_usb_ww
?
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.
Difference between ti_usb_ww0 and ti_usb_ww1 was the way the python script oref0-subg-ww-radio-parameters.py was called. refactored it to use --ww_ti_usb_reset=[yes|no] for WW-pump which is much cleaner
bin/oref0-setup.sh
Outdated
@@ -169,6 +173,19 @@ if [[ -z "$DIR" || -z "$serial" ]]; then | |||
# Force uppercase, just in case the user entered ww | |||
radio_locale=${radio_locale^^} | |||
|
|||
# check if user has a TI USB stick and a WorldWide pump and want's to reset the USB subsystem during mmtune if the TI USB fails | |||
ti_usb_ww0="no" # assume you don't want it by default | |||
ti_usb_ww1="" |
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.
Should we rename this to something more descriptive, like ti_ww_reset or something?
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.
refactored to ww_ti_usb_reset
bin/oref0-setup.sh
Outdated
if [[ $REPLY =~ ^[Yy]$ ]]; then | ||
ti_usb_ww="yes" | ||
else | ||
ti_usb_ww="no" |
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.
Having both upper and lowercase versions of the same variable name is very confusing. Can we rename them so it's clear what's being done?
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 only use ww_ti_usb_reset now
bin/oref0-setup.sh
Outdated
@@ -276,6 +299,7 @@ if [[ ! -z "$min_5m_carbimpact" ]]; then | |||
fi | |||
if [[ ! -z "$ENABLE" ]]; then echo -n " --enable='$ENABLE'" | tee -a /tmp/oref0-runagain.sh; fi | |||
if [[ ! -z "$radio_locale" ]]; then echo -n " --radio_locale='$radio_locale'" | tee -a /tmp/oref0-runagain.sh; fi | |||
if [[ ! -z "$ti_usb_ww" ]]; then echo -n " --ti_usb_ww='$ti_usb_ww'" | tee -a /tmp/oref0-runagain.sh; fi |
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.
It looks like you're defaulting this variable to "no" instead of leaving it null? In that case, this will always be included in runagain for non-WW users, which probably isn't what we want.
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.
fixed. Changed to if [[ $ww_ti_usb_reset =~ ^[Yy]$ ]]; then echo -n " --ww_ti_usb_reset='$ww_ti_usb_reset'" | tee -a /tmp/oref0-runagain.sh; fi
bin/oref0-setup.sh
Outdated
@@ -526,12 +550,26 @@ else | |||
openaps alias add wait-for-silence '! bash -c "(mmeowlink-any-pump-comms.py --port '$ttyport' --wait-for 1 | grep -q comms && echo -n Radio ok, || openaps mmtune) && echo -n \" Listening: \"; for i in $(seq 1 100); do echo -n .; mmeowlink-any-pump-comms.py --port '$ttyport' --wait-for 30 2>/dev/null | egrep -v subg | egrep No && break; done"' | |||
openaps alias add wait-for-long-silence '! bash -c "echo -n \"Listening: \"; for i in $(seq 1 200); do echo -n .; mmeowlink-any-pump-comms.py --port '$ttyport' --wait-for 45 2>/dev/null | egrep -v subg | egrep No && break; done"' | |||
if [[ ${radio_locale,,} =~ "ww" ]]; then | |||
if [ -d "$HOME/src/subg_rfspy/" ]; then | |||
echo "$HOME/src/subg_rfspy/ already exists; pulling latest" | |||
(cd ~/src/subg_rfspy && git fetch && git pull) || die "Couldn't pull latest subg_rfspy" |
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.
We should use $HOME
instead of ~
here.
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 copied it from your git clone oref0 code :) . Fixed on all parts within oref0-setup.sh
bin/oref0-setup.sh
Outdated
(cd ~/src/subg_rfspy && git fetch && git pull) || die "Couldn't pull latest subg_rfspy" | ||
else | ||
echo -n "Cloning subg_rfspy: " | ||
(cd ~/src && git clone https://github.com/ps2/subg_rfspy) || die "Couldn't clone oref0" |
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.
And here.
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.
there were 9 occurences in your code. Fixed
bin/oref0-setup.sh
Outdated
else | ||
ti_usb_ww1="--resetpy" | ||
fi | ||
sed -i"" 's/^\(mmtune.*\); \(echo -n .*mmtune:\)/\1; echo -n subg-ww-radio-parameters:; oref0-subg-ww-radio-parameters.py '$ti_usb_ww1' -v ; \2/g' openaps.ini |
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.
Would it be possible to redefine the alias directly with openaps
commands instead of using sed
?
Fixed all review comments of @scottleibrand and implemented |
@PieterGit if you can resolve the merge conflicts and verify that this is ready for testing in dev, I can merge when you're ready. |
With the fixes I just pushed, this is now installing correctly without errors on an Explorer board rig with oref0-runagain.sh Still need to try an interactive setup and make sure it doesn't present any confusing new questions for non-WW users. |
…comms.py to oref0-init-pump-comms" This reverts commit fda9a9b.
Python names need |
bin/oref0-setup.sh
Outdated
@@ -353,7 +353,6 @@ else | |||
(cd $HOME/src && git clone git://github.com/openaps/oref0.git) || die "Couldn't clone oref0" | |||
fi | |||
echo Checking oref0 installation | |||
cd $HOME/src/oref0 |
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.
We need to re-add this.
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.
i'm back on my old 1.5 openaps rig, got waiting for 30-40 min on the devww |
@libxmike : are you sure you have the latest devww checkout, the python scrips, should have underscores Most of the
To install this branch use:
See: https://gitter.im/nightscout/intend-to-bolus?at=58d6cb3ab402a53211b07271 |
@libxmike To debug your error please use commands such as:
What does the @scottleibrand : i think this branch is ready and that @libxmike is finding a different issue (not caused by changes on my branch). Is there anything needed before this can be merged to dev? |
@scottleibrand merged this with 0af48b8 |
Oops, that was accidental. I think it was ready to go, though, and I haven't seen any problems with it since merging, so I guess we're good to test further in dev. |
Conflicts: bin/oref0-setup.sh
My accidental merge didn't get all of our fixes, so reopening this PR to merge those. |
dont know whats wrong with it, installed last dev but getting error, /dev/ttyACM0 is gone after a while, with same Ti USB Stick in my older dev rig (same hardware, Rpi 3) it is working all the time
|
@libxmike did you enable the WW_TI_USB option in the setup script. You check if you have We might have found the root cause of this issue, see #445 This pull request has been merged. So please create a new issue if you keep having problems with current dev checkout. |
@PieterGit thx for that, looks like I dont have the
after adding |
@libxmike I must look into that, but won't be until next week. But yes, the setup-script should add the |
This pull request implements
#354
Tested on my RPI3.