Pinoccio / chrome-app-pinoccio

1 stars 4 forks source link

Consume and return results for wifi functions #13

Closed jingman closed 10 years ago

jingman commented 10 years ago
amcjen commented 10 years ago

Temas, cool if I assign this to you, to make sure all these commands work correctly with the Chrome app?

temas commented 10 years ago

Totally

Eric Jennings wrote:

Temas, cool if I assign this to you, to make sure all these commands work correctly with the Chrome app?

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/pinoccio-chrome-app/issues/13#issuecomment-32392151.

amcjen commented 10 years ago

Awesome. When you get those responses above working with the chrome app, just check 'em off up there so Jake knows they're good to go.

Thx!

amcjen commented 10 years ago

References #12 as well, closing that one

temas commented 10 years ago

The latest commit allows for all of these to work on both the current public release and the beta release of Chrome. There is an issue that requires us to use 38400 bitrate over serial right now. I've filed a Chrome bug about it at https://code.google.com/p/chromium/issues/detail?id=335961

matthijskooijman commented 10 years ago

How can we use 38400 over serial? Doesn't that mean we have to change both the bootloader and the sketch to use the lower bitrate? And especially for the bootloader we don't want that, as it'll permanently make sketch uploads three times slower?

amcjen commented 10 years ago

OOOOOOHHHH. I forgot about the bootloader needing 115200. Yeah, this isn't going to work. Damn it.

So our options at this point are:

temas commented 10 years ago

To be clear, this issue is explicitly on Chrome Beta. We could ship with the assumption of Chrome public release right now, but as soon as beta gets updated to public, we're boned.

amcjen commented 10 years ago

Ah, interesting. What about this then?

We ship with Chrome public working at 115200, and we flash the boards via bootloader @ 115200 as planned. Then one the following two things happens:

No idea at all when chrome stable will roll out huh?

matthijskooijman commented 10 years ago

There code be a third option: Suggest that people use an older version of Chrome for the provisioning process.

Does this bug occur on all platforms, or is it MacOS specific?

Did we report that bug to chromium, or was it already reported by someone else? If the latter, we should leave a comment that we can also reproduce the bug...

amcjen commented 10 years ago

True, there is that option @matthijskooijman . Good point.

Temas posted the bug linked above. The code in Chromium that is preventing this from working seemed to be hardwired to OSX (and Linux IIRC), via #defines

amcjen commented 10 years ago

I also reached out to a contact we have on the Chrome team. Not sure it'll get us anywhere, but worth a shot.

jingman commented 10 years ago

I think that the explicit bitrate passthrough for MACOSX was just not updated as part of this commit (old L91, new L62). Hopefully that means that there has not been an explicit limiting of the mac osx bitrate, it's just that the passthrough/else for MACOSX wasn't updated. Hopefully we can get some eyes on this and get it fixed!

matthijskooijman commented 10 years ago

Seems like that commit doesn't actually change anything? There is still the:

66 default:
67 return bitrate_;

in the new getBaudRate() function, which gets called and passed to the bitrate setting stuff?

    81 int bitrate_opt_ = getBaudRate(bitrate_);
    82 
    83 cfsetispeed(&options, bitrate_opt_);
    84 cfsetospeed(&options, bitrate_opt_);
amcjen commented 10 years ago

Perhaps the SetCustomBitrate() needs to be called, per this issue?

jingman commented 10 years ago

@matthijskooijman Yep, was looking at it wrong, shoot!

temas commented 10 years ago

I'm not seeing an obvious way to call this quickly. I could try calling an update on the open connection, but by then it has already caused the board to boot at the wrong bitrate and we could have missed things. I'm only quickly scanning right now though. If any of you have a bit more time, before this evening, to look at it some help would be appreciated.

Jake Ingman mailto:notifications@github.com January 21, 2014 at 11:09 AM

@matthijskooijman https://github.com/matthijskooijman Yep, was looking at it wrong, shoot!

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/pinoccio-chrome-app/issues/13#issuecomment-32912049.

Jake Ingman mailto:notifications@github.com January 14, 2014 at 2:53 PM

  • |wifi.list| - returns a multiline response over time.
  • |wifi.config("ssid","password","ip",port)| - returns nothing if it succeeds.
  • |wifi.connect| - returns |Wi-Fi backpack connecting...|, then pauses, then |Done| if it worked, or an error if it didn't (use bogus wifi.config info to test).

— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/pinoccio-chrome-app/issues/13.

matthijskooijman commented 10 years ago

You were testing version 33.1750, for which I think the source is here: http://src.chromium.org/viewvc/chrome/branches/1750/src/chrome/browser/extensions/api/serial/serial_connection_posix.cc?revision=HEAD

Looking at that code, it seems they restructured it more, there is now a BitrateToSpeedConstant() function, that returns false for bitrates without constants. A bit further down in SerialConnection::ConfigurePort, where BitrateToSpeedConstant is callled, it seems that if it returns false, SetCustomBitrate is called (which calls ioctl(file, IOSSIOSPEED, &speed) instead of the behaviour shown in the previously linked code, where it just calls cfsetispeed and cfsetospeed with the raw bitrate value.

For comparison, here's what the code looked like in (what I think is) the latest 32.x version: http://src.chromium.org/viewvc/chrome/branches/1700_39/src/chrome/browser/extensions/api/serial/serial_connection_posix.cc

Looking at the history for the trunk version, no further changes have been made here.

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/serial/serial_connection_posix.cc?view=log

I'm not sure what the proper way to handle this in C code is, but it looks like the above changes broke things?

Given that the custom bitrate code is supposed to handle this case now, perhaps you could leave a comment at https://codereview.chromium.org/107363002 or http://code.google.com/p/chromium/issues/detail?id=243097 to get the attention of the right folks?

matthijskooijman commented 10 years ago

Heh, took me a while to find this issue again, since the serial speed stuff was quite off-topic. In any case, it seems the Chrome folks have sneakily been discussing the speed issue in another bug report: http://code.google.com/p/chromium/issues/detail?id=337482

The good news is that they've come up with a fix, but the bad news is that it seems the fix will be released in version 34, so 33 will be released broken.

In any case, @temas, you should probably check out the fix and let them know if it works again as expected.