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

Adding @nplus11's work on Rust init #292

Merged
merged 1 commit into from
Jun 16, 2016
Merged

Adding @nplus11's work on Rust init #292

merged 1 commit into from
Jun 16, 2016

Conversation

johnnyman727
Copy link
Contributor

@johnnyman727 johnnyman727 commented Sep 10, 2015

99% of this code was written by @nplus11 in #131 but bit rot has forced me to rebuild the PR manually.

  • The Rust code needs to be fixed
  • It needs unit tests

fn main() {

// Set the led pins as outputs with initial states
let led1 = tessel.led[0].output(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I try to compile I get this error:

➜  t2-rust  cargo build
    Updating git repository `https://github.com/tessel/rust-tessel.git`
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading unix_socket v0.3.2
 Downloading libc v0.1.10
   Compiling debug-builders v0.1.0
   Compiling libc v0.1.10
   Compiling unix_socket v0.3.2
   Compiling rust_tessel v0.0.1 (https://github.com/tessel/rust-tessel.git#8b0c9aba)
   Compiling blinky v0.0.1 (file:///Users/Jon/Code/t2-rust)
src/main.rs:7:14: 7:20 error: unresolved name `tessel`
src/main.rs:7   let led1 = tessel.led[0].output(1);
                           ^~~~~~
src/main.rs:8:14: 8:20 error: unresolved name `tessel`. Did you mean `led1`?
src/main.rs:8   let led2 = tessel.led[1].output(0);
                           ^~~~~~
error: aborting due to 2 previous errors
Could not compile `blinky`.

Copy link
Member

Choose a reason for hiding this comment

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

The example code doesn't match the API of the rust-tessel lib, and also the API couldn't actually work quite like that.

It uses tessel as if there were a variable named tessel holding a struct with an led field, but extern crate rust_tessel as tessel; only brings in a tessel module. Modules and variables are in separate namespaces. Unlike in JS, Rust modules are just collections of function/structure/trait/constant/etc definitions, and don't contain object instances or run initialization code just by including them (global variables are possible, but global mutable state is highly discouraged).

You could do:

let tessel = tessel::new();
tessel.led[0].output(1);

or

tessel::led(0).output(1)

(assuming that the library implemented LEDs with one of those APIs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, @kevinmehall.

It looks like it won't be quite so easy because I don't think the tessel lib was implemented in such a way that you could call new on it. It probably needs updating to newer versions of Rust as well.

@Frijol
Copy link
Member

Frijol commented Sep 11, 2015

tagging @flaki in case you haven't seen this

@johnnyman727
Copy link
Contributor Author

Dependent on tessel/tessel-rust#4 merging.

@johnnyman727
Copy link
Contributor Author

johnnyman727 commented May 11, 2016

@rwaldron this was rebased on your branch and should be ready for review.

I was admittedly light on tests for JavaScript and Python because I didn't change the existing (and working) JavaScript code much and I didn't add any functionality for Python besides printing a sad face. We actually didn't have any tests for init code before so now if any problems arise, we have the infrastructure in place to add it.

@johnnyman727
Copy link
Contributor Author

@rwaldron ping. Not sure if the Appveyer fails are real because it's having issues with controller.js on some versions of Node which I didn't touch.

@johnnyman727
Copy link
Contributor Author

Rebased on master again @rwaldron. It should be ready to go.

@rillian
Copy link

rillian commented May 31, 2016

Ping on this. We got a tessel for testing today and would really like to experiment with rust on it.

The travis failure looks like an issue with the usb mock. Is that likely to be from this patch?

@johnnyman727
Copy link
Contributor Author

@flaki if you're around can you take a look at this?

@johnnyman727
Copy link
Contributor Author

@rillian the USB error is expected since the VM doesn't have any USB controllers (it won't fail the test). Now we have some other failing tests due to an outdated SSL cert on the build server but hopefully my latest commit fixes that by mocking requests (as it should have been in the first place).

@flaki
Copy link

flaki commented Jun 2, 2016

Hey! The code looks okay upon skimming through it.
I have my tessel workshorse (that is, my Mac) at the Airbnb, will have a look at this and the test later today.

@johnnyman727 johnnyman727 force-pushed the jon-init-rust branch 2 times, most recently from 0e4e28b to a93a7af Compare June 3, 2016 06:30
@rillian
Copy link

rillian commented Jun 3, 2016

Can someone with permissions his the rebuild button here too?

@rwaldron
Copy link
Contributor

rwaldron commented Jun 3, 2016

Done!

@rillian
Copy link

rillian commented Jun 3, 2016

Thanks! All green now.

@flaki
Copy link

flaki commented Jun 5, 2016

So, I'm signing off on this one to get the ball rolling - code looks fine to me so far, but there are some issues we need to fix to be able to make actual use of this stuff.

What I've ran across:

  • We cannot run/push the inited module in current state
  • Even if we could, blinky is not working currently (according to Jon)
  • (unrelated) t2 init -i (interactive mode) throws an error

We should follow up with those, but otherwise good to go!

@rillian
Copy link

rillian commented Jun 12, 2016

With this patch, t2 init --lang rust generates a reasonable rust starting structure. I agree with @flaki this is ready to merge.

@rwaldron rwaldron force-pushed the master branch 2 times, most recently from 1aa40f6 to c37ff9d Compare June 14, 2016 20:57
@johnnyman727
Copy link
Contributor Author

johnnyman727 commented Jun 16, 2016

Thanks for looking this over @flaki and @rillian! I have confirmed that the t2 init -i error is an existing issue with JavaScript init and unrelated to this PR. I will open a separate issue. I have rebased on master and pushed my latest changes. If it passes CI, I'll go ahead and merge.

@johnnyman727
Copy link
Contributor Author

This is passing all tests except an unrelated test on Node 6 which isn't supported yet. Additionally, other branches seem to be hitting this with makes me believe it's an issue that is already existing in master. With that in mind, I'm going to go ahead and merge.

@johnnyman727 johnnyman727 merged commit 1ba5408 into master Jun 16, 2016
@johnnyman727 johnnyman727 deleted the jon-init-rust branch June 16, 2016 15:27
@johnnyman727
Copy link
Contributor Author

And master is passing CI after merge so I'm not entirely sure what was up with that.

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.

None yet

6 participants