Skip to content

Conversation

@PieterGit
Copy link
Contributor

@PieterGit PieterGit commented Aug 19, 2018

Work in progress / do not merge yet.

The uploading of cgm data with G4-go does not work for me with tokenbased authentication. Working on that.

@scottleibrand can you do a review so far?

PieterGit and others added 12 commits August 18, 2018 14:26
- upgrade bluez to 5.50 (if < 5.43 is available)
- remove spaces at the end of lines
- auto install bluez-tools (add -y flag)
- show latests versions of ecc1 libraries as example
- remove duplicate buildgofromsource line
- fix .profile warning
- use Mhz notation for medtronic_frequency.ini
you should instead use G4-Go which is much better alternative
use G4-Go instead
only ask for ww_ti_usb_edison on non edison hardware

append radio_locale to pump.ini
the other find gave an empty set on edison
@jpcunningh
Copy link
Contributor

@PieterGit, will you incorporate the changes youn requested in this PR such that I should close the other 2 PRs?

Thanks!

Jeremy Cunningham and others added 6 commits August 20, 2018 09:19
Copy link
Contributor

@scottleibrand scottleibrand left a comment

Choose a reason for hiding this comment

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

A few comments inline.

# if the rig doesn't recover after that, reboot:
oref0-radio-reboot &
# DEPRECATED WITH 0.7.0. See if we can do without
# oref0-radio-reboot &
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this wasn't deprecated: I re-enabled it for 0.7.0 with commit de462de

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 see a lot of radio reboots with the 0.7.0 that seem unnecessary and cause interruption during normal pump-rig comms. if it's really needed in some corner cases perhaps move to every 15 minutes? I would like to make it configurable and see if I can do without these reboots. I think there is still some other issue that makes rebooting without network not boot up the rig without problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to lengthen the delay before reboot so it has slightly longer to start making progress and cancel the scheduled reboot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only lengthen the delay will keep a lot of false positives (that hopefully will be cancelled in time). previouse mmweowlink and pump comms could really be messed up after a reboot. I rather see if we can minimize the amount of reboots, perhaps only make them as an option.

How about testing it every 15 minutes and upping the reboot time to 10 minutes. That would give a minimum of 10 minutes and max of 25 minutes in case of pump-rig comms failures. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen any issues caused by a reboot: can you provide more specifics on what problems you've seen as a result of rebooting a 0.7.0 rig?

I'd rather not wait 25 minutes to start looping again if the rig gets into a bad state. I'm ok with a few extra log messages about scheduled reboots that get canceled before occurring: those don't do any harm, and if someone is looking at their logs closely enough to notice them, they might point to an underlying issue...

Copy link
Contributor Author

@PieterGit PieterGit Sep 10, 2018

Choose a reason for hiding this comment

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

@scottleibrand
Issues I know of are:

  • older explorer boards have issues with Dexcom G4 USB and the micro usb needs to be inserted after reboot
  • edison does not have RTC and during systemstartup internet connection is required in order to set the right time. We should make sure the time from the pump is used in those cases, but that doesn't completely work (yet)

Another reason is that rebooting in case of an issue instead of fixing the root cause is a Microsoft way of doing stuff.

Can we agree on:

  • making reboot configurable
  • let's try to disable it by default and see if the current 0.7.x codebase has issues that resolve if you reboot the rig
  • try to resolve those issues, instead of rebooting
  • people who enable the setting should check if the 0.7.0-dev rig reboots in common daily use
  • before 0.7.x release decide if we need to use enable reboot by default

"moment": "^2.14.1",
"request": "^2.79.0",
"share2nightscout-bridge": "^0.1.5",
"share2nightscout-bridge": "git://github.com/nightscout/share2nightscout-bridge.git#wip/generalize",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

@PieterGit PieterGit Aug 20, 2018

Choose a reason for hiding this comment

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

This is the version Nightscout uses. We should re publish a new NPM package. I tried contacting Ben West, but didn't get a response.
For a list of changes see https://github.com/nightscout/share2nightscout-bridge/commits/wip/generalize
It fixes a security issue.

}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this added intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, adding a npm-shrinkwrap.json makes package integrity better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does npm update it automatically and create merge conflicts? Or is it maintained manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's changed with the npm commands. i did not have a lot of merge conflicts with it for nightscout. In case you have merge problems, you always re-create it with npm shrinkwrap

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem we've seen is that people do a git pull and npm run global-install, and then the next time they try to git pull, they get merge conflicts with files that were updated locally by npm, and also updated and pushed to the repo by someone else. So given the way we're having normal non-developer users use npm commands to install dev versions of oref0, I'd rather not have any files present that are automatically updated by npm on the rigs of non-developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed npm-shrinkwrap.json. The proper way is not using npm run global-install and using a published npm package, but that's quite some work to migrate to that.

@PieterGit PieterGit changed the title various fixes (mainly WW related) from fresh edison install various fixes (mainly WW related) from fresh edison/rpi0 install Aug 27, 2018
#if ! sudo apt-get install -y npm; then
# install/upgrade to node 8
if ! nodejs --version | grep 'v8.'; then
if ! nodejs --version | grep 'v8.11'; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Won’t this break when 8.12 comes out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, changed to

# install/upgrade to latest node 8 if neither node 8 nor node 10+ LTS are installed
if ! nodejs --version | grep -e 'v8\.' -e 'v1[02468]\.' ; then

PieterGit and others added 13 commits September 8, 2018 15:32
…grade

Resolved Conflicts:
#	bin/oref0-pump-loop.sh
#	bin/oref0-setup.sh
…nto 201808_upgrade

# Conflicts:
#	bin/oref0-setup.sh
…in preferences.json, fix bug in .cgm_loop_path for CGM=g4-go, upgrade golang

- improved the text for the Go libs, making it more clear that D will use a released version, and otherwise you need a tags
- (temporary) remove  .nightscout_api_secret because it's unhashed and because it's not yet used,  see discussion https://gitter.im/openaps/oref0?at=5ba653d6f7e15806250b8c7b
- fix bug that cgm_loop_path was wrong for g4-go
- upgrade golang from 1.9 to 1.11
@scottleibrand
Copy link
Contributor

Fixed merge conflicts and made a few other small changes in https://github.com/openaps/oref0/compare/dev...PieterGit-201808_upgrade?expand=1 to get it to the point where I think it's ready to merge. @PieterGit is this/that ready to merge from your perspective?

@scottleibrand
Copy link
Contributor

scottleibrand commented Oct 7, 2018

Testing that, I get:

npm ERR! missing: request@^2.79.0, required by [email protected]
npm ERR! missing: share2nightscout-bridge@git://github.com/nightscout/share2nightscout-bridge.git#wip/generalize, required by [email protected]
npm ERR! missing: yargs@^12.0.1, required by [email protected]
npm ERR! error in /usr/lib/node_modules/oref0/node_modules/wrap-ansi: ENOENT: no such file or directory, open '/root/src/oref0/node_modules/wrap-ansi/package.json'
npm ERR! invalid: [email protected] /usr/lib/node_modules/oref0/node_modules/share2nightscout-bridge
npm ERR! invalid: [email protected] /usr/lib/node_modules/oref0/node_modules/yargs

Investigating...

@scottleibrand
Copy link
Contributor

The first time I ran oref0-runagain, it errored out completely, complaining about missing packages and files. Re-running it, it still got a bunch of complaints about share2nightscout-bridge and yargs, but it continued past them successfully. On the third run, it didn't complain at all. Not sure if there's some order-of-operations bug here or what: we'll have to keep an eye on what happens on additional rigs.

@scottleibrand scottleibrand mentioned this pull request Oct 8, 2018
@scottleibrand
Copy link
Contributor

Looks like the latest yargs is incompatible with node 0.10:

/root/src/oref0/node_modules/yargs/index.js:5
const yargs = require('./yargs')
^^^^^
SyntaxError: Use of const in strict mode.
at Module._compile (module.js:439:25)
at Object.Module._extensions..js (module.js:474:10)
at Module.load (module.js:356:32)
at Function.Module._load (module.js:312:12)
at Module.require (module.js:364:17)
at require (module.js:380:17)
at Object. (/root/src/oref0/bin/oref0-get-profile.js:42:16)
at Module._compile (module.js:456:26)
at Object.Module._extensions..js (module.js:474:10)
at Module.load (module.js:356:32)
Could not run oref0-get-profile

So I just updated #1136 to force upgrade to node 8+

@scottleibrand
Copy link
Contributor

Closing in favor of #1136

@PieterGit PieterGit deleted the 201808_upgrade branch December 25, 2018 08:37
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.

3 participants