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

Bleat required regardless of BLE flag in CLI #37

Merged
merged 4 commits into from
Oct 10, 2016

Conversation

chalkers
Copy link
Contributor

@chalkers chalkers commented Oct 1, 2016

Problem:

On Windows, when a non-compatible USB Bluetooth 4.0 wasn't found, it caused a massive stage trace to appear regardless if you wanted to use ble flag.

Error: No compatible USB Bluetooth 4.0 device found!
    at BluetoothHciSocket.bindUser (C:\Users\andre\Projects\test_craig\node_modules\noble\node_modules\bluetooth-hci-socket\lib\usb.js:70:11)
    at BluetoothHciSocket.bindRaw (C:\Users\andre\Projects\test_craig\node_modules\noble\node_modules\bluetooth-hci-socket\lib\usb.js:28:8)
    at Hci.init (C:\Users\andre\Projects\test_craig\node_modules\noble\lib\hci-socket\hci.js:99:35)
    at NobleBindings.init (C:\Users\andre\Projects\test_craig\node_modules\noble\lib\hci-socket\bindings.js:83:13)
    at new Noble (C:\Users\andre\Projects\test_craig\node_modules\noble\lib\noble.js:50:18)
    at Object.<anonymous> (C:\Users\andre\Projects\test_craig\node_modules\noble\index.js:4:18)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)

The problem was that bleat was always being "tried" to be included outside of the checkInit() thus always being required and that the defaultValue was always true.

Solution:
Set defaultValue to be false and include bleat only when necessary.

I also did a version bump to 0.0.21.

@gfwilliams
Copy link
Member

Can we not just trap this exception and carry on? This will get used a lot more with puck.js, and I'd rather not have to make people explicitly add --ble every time they use the ide or command-line tools

@chalkers
Copy link
Contributor Author

chalkers commented Oct 1, 2016

The require('bleat') is already in a try so I don't know how you'd trap it. It also feels it's further down in the stack. The CLI (even though it works) exits with exit code 1.

If this isn't the desired behavior of the --ble flag then it should be removed from the CLI or have the option to switch it off. I tried --ble false and --ble=false and it doesn't do anything...

Either way the pull request fixes the behavior that's described in --help. I guess the question is, do you want a massive stack trace to appear for Windows users of the non-Puck.js users?

@gfwilliams
Copy link
Member

Yeah, the --ble flag should ideally get modified to allow you to turn it BLE off.

So the CLI actually works fine? I'm not sure why the -1 error code, but that could be because it hit some other snag later down (like not finding a port?).

Are you sure it's not just the console.error that gets called with the exception in the catch block? That looks trivial to convert to a .log, which then gets hidden unless --verbose is called.

@gfwilliams
Copy link
Member

gfwilliams commented Oct 5, 2016

Any thoughts on this? Is it likely to just be the console.error?

@chalkers
Copy link
Contributor Author

chalkers commented Oct 5, 2016

I'll take a look soon! Been a little busy!

@chalkers
Copy link
Contributor Author

chalkers commented Oct 7, 2016

Hi there,

There still was a stack trace without the console.error. If you check the last commit, I've changed the --ble to --no-ble now.

0267ec9

So doing --no-ble jumps over the initialization now.

@gfwilliams
Copy link
Member

Great, thanks!

@gfwilliams gfwilliams merged commit a625096 into espruino:gh-pages Oct 10, 2016
@gfwilliams
Copy link
Member

I'll add an issue about the stack trace though - I'll have to look into why that's happening

@chalkers
Copy link
Contributor Author

Can you ping me when you publish the module? :)

@gfwilliams
Copy link
Member

Ping!

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.

2 participants