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

Fixed invalid syntax of function declaration #14

Merged
merged 3 commits into from Nov 14, 2015

Conversation

@pastaclub
Copy link
Contributor

pastaclub commented Nov 14, 2015

No idea how this got in there, but the syntax is invalid. I guess it should have been a function.

@pastaclub

This comment has been minimized.

Copy link
Contributor Author

pastaclub commented Nov 14, 2015

And regarding openSerial: on my system for some reason portToDevice is an empty array. That caused the code to crash on accessing undefined.open, because the check for the existence of the port was wrong.

@pastaclub

This comment has been minimized.

Copy link
Owner Author

pastaclub commented on 764025f Nov 14, 2015

The "!" is stronger than the "in" operator, so the condition was actually equivalent to
((!serialPort) in portToDevice) instead of the intended (!(serialPort in portToDevice).
Once this is fixed, the next check is no longer needed.

@gfwilliams

This comment has been minimized.

Copy link
Member

gfwilliams commented Nov 14, 2015

The function syntax is ES6 function syntax - all the Web Bluetooth code uses it and some got left in... but anything that supports Web Bluetooth should support ES6, so I'm surprised it caused you errors?

Also, the serial code definitely does work, so I'm not sure the positioning of ! is an issue?

What system are you running on? and are you using it as a chrome extension?

If portToDevice is an empty array, presumably that means that no serial ports have been found? You could go into settings, enable the 'seruial over audio' and see if you can connect with that.

@pastaclub

This comment has been minimized.

Copy link
Contributor Author

pastaclub commented Nov 14, 2015

I'm running espruinotool on the command-line. If it uses the globally installed node.js, then that would be v0.10.26.


The problem with the ! without the extra brackets (that I added) is that it negates only the string serialPort. Therefore the error check is not working and the code crashes later on when trying to access the function open of an undefined variable currentDevice. Look at this:

$ node
> serialPort="foo"; portToDevice=[];
[]
> if (!serialPort in portToDevice) {console.log('Port not found');}
undefined
> if (!(serialPort in portToDevice)) {console.log('Port not found');}
Port not found
undefined

node on my machine doesn't eat that function declaration:

$ node -e "fn = (error => {console.log('error');});"

[eval]:1
fn = (error => {console.log('error');});
             ^
SyntaxError: Unexpected token >
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:532:25)
    at startup (node.js:80:7)
    at node.js:902:3
$ node -e "fn = (function(error) {console.log('error');});"
$

Besides, this is the ONLY place in the repo where a function is declared by that syntax. There's a whole sequence of lines in serial_web_bluetooth.js which declare functions in the old fashion and just a few lines below there is this one arrow function declaration. That's also incosistent (adding to the fact that it doesn't compile on systems like mine).

gfwilliams added a commit that referenced this pull request Nov 14, 2015
Fixed invalid syntax of function declaration
@gfwilliams gfwilliams merged commit 5bd7728 into espruino:gh-pages Nov 14, 2015
@gfwilliams

This comment has been minimized.

Copy link
Member

gfwilliams commented Nov 14, 2015

Very true - sorry, had to head out earlier so didn't merge straight away.

So you have node-serialport, and you're still not seeing any ports?

@pastaclub

This comment has been minimized.

Copy link
Contributor Author

pastaclub commented Nov 14, 2015

The dependency serialport was installed by npm:

EspruinoTools dennis$ npm list |grep serialport
├─┬ serialport@1.2.5

but true, I don't see any ports :(

I just updated my old node.js to 5.0.0, but now it crashes on another dependency...

@pastaclub

This comment has been minimized.

Copy link
Contributor Author

pastaclub commented Nov 14, 2015

I found what the problem was. The code of espruinotool (index.js) used to work this way: when no port is specified, it scans for ports and then connects to the first port found. That works fine. But if a port was specified, it does NOT scan for ports, it tries to connect right away. However the function openSerial in serial.js checks whether the specified port exists (by checking whether the string is an element of the array portToDevice). portToDevice is a variable in a higher scope and in this case an empty array, because the function getPorts that fills it has never been called.

I fixed it here:
pastaclub/espruino-tools@6586390
and now it connects to the port, but I think the code is still not clean, because it should not be the responsibility of the instantiating code (WebIDE or espruinotool) to make sure that getPorts gets called first because openSerial relies on it due to the shared variable.

Please let me know what you think.

@gfwilliams

This comment has been minimized.

Copy link
Member

gfwilliams commented Nov 16, 2015

Thanks - yes, sounds to me like serial.js should know if getPorts hasn't been called and should do it automatically. I'll make some changes to fix that in a bit.

@gfwilliams

This comment has been minimized.

Copy link
Member

gfwilliams commented Nov 16, 2015

Ok, just done...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.