NOP0 / rustmatic

PLC programming in Rust!
Apache License 2.0
35 stars 2 forks source link

Tinkering #37

Closed NOP0 closed 4 years ago

NOP0 commented 4 years ago

This is not a PR :slightly_smiling_face:

Is this at all like what you had in mind with respect to adding wasm to runtime?

NOP0 commented 4 years ago

So, now theres the subject of instantiating this WasmProcess. I've added

impl WasmProcess {
    pub fn new(program: WasmProgram) -> WasmProcess { WasmProcess { program } }
}

Which means I eat the program. Is that ok?

NOP0 commented 4 years ago

@Michael-F-Bryan By running cargo test -- --nocapture in rustmatic::runtime we'll now get "Polling" from the wasm program log function all the way via the runtime and to stdout. This is cool, seems that the SystemEnvironment adapter works.

However, there's no real test here, since I'm not sure how I would capture and assert the println! to stdout. Suggestions?

I guess the get/set variable and read/write inputs/outputs methods will be easier to test, since these will affect the data structures in the runtime itself.

Michael-F-Bryan commented 4 years ago

The runtime should probably use a Box<dyn Write> that we write to instead of using println!(), defaulting to Box<std::io::Stdout>. That way we can redirect it to some Vec<u8> in memory while testing or a socket if it's running on a remote device.

As usual, adding another layer of indirection fixes most problems :grin:

NOP0 commented 4 years ago

@Michael-F-Bryan The "Polling" test works now.

To get to the "Blinky" MVP with respect to the runtime, I also need to implement the rest of the functions in impl Environment for SystemEnvironment. I don't see any potential issues with these now that the POC has been shown to work.

How do you feel about the PR so far, should I push the missing parts to this PR or should this PR be merged into master now, with the rest as smaller PR's?

Michael-F-Bryan commented 4 years ago

How do you feel about the PR so far, should I push the missing parts to this PR or should this PR be merged into master now, with the rest as smaller PR's?

It might be easier to break things into small chunks. Small discrete chunks of functionality are easier to review and merge, plus it feels better to know you've merged a dozen successful PRs instead of still be waiting for a really big PR to go through review.

NOP0 commented 4 years ago

@Michael-F-Bryan I'm not sure about the etiquette regarding this, should I as a collaborator just merge this, or should you as owner approve it? Sorry for probably being over-catious 😄

Michael-F-Bryan commented 4 years ago

We tend to ask each other to review changes and then keep refining until we're happy, so I think it's fine for you to merge.

It also means if one of us is AFK for a while it doesn't block the other while they're waiting for the "Merge" button to be pressed.