Skip to content

Add evaluation guidelines and instructions on how to submit a project #6

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

Merged
merged 2 commits into from
Feb 20, 2019

Conversation

japaric
Copy link
Member

@japaric japaric commented Jan 10, 2019

The guidelines include all I can think of at the moment. We'll probably come up
with more of them once start reviewing user submissions. And, maybe, in the
future these guidelines could become a document on "good practices for
structuring embedded applications".

Q: Is there any additional information we'll like to request from the submitter,
apart from the information in data.yml (see #5)? This information would be
provided in the description of the pull request. If not then we don't need a
pull request template as I suggested in #3.

r? @rust-embedded/resources

closes #3
closes #4

@japaric japaric mentioned this pull request Jan 10, 2019
adamgreig
adamgreig previously approved these changes Jan 10, 2019
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
`mem::transmute`, unchecked creation of singletons and unchecked use of
`static mut` variables are huge red flags.

- Explicit panicking (e.g. `unwrap`) is used instead of proper error handling.

Choose a reason for hiding this comment

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

Uhm, what is an explicit panic? There're valid uses of unwrap() and in many cases there's simply no other way of properly handling an error other than to panic.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could ask for expect("bla bla") instead of unwrap() which at least gives a reason for the panic, though there might be binary size concerns...

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhm, what is an explicit panic?

unwrap, expect, etc.

Compare:

Explicit panicking

fn on_next_packet(bytes: &[u8]) {
    let packet = Packet::parse(bytes).unwrap();

    // do stuff
}

vs

Proper error handling (robust code)

fn on_next_packet(bytes: &[u8]) {
    if let Ok(packet) = Packet::parse(bytes) {
        // do stuff
    } else {
        // we got junk
        #[cfg(debug_assertions)] // if in `dev` mode
        error!("malformed packet of {} bytes", bytes.len()); // log an error message
    }
}

There're valid uses of unwrap()

IMO, unwrap should only be used when you know the code won't panic
at runtime
. For example, Resut::<T, !>.unwrap is OK because there's no
alternative API.

in many cases there's simply no other way of properly handling an error other than to panic.

IMO, jumping to a "fail-safe" state when unrecoverable I/O errors happen is preferable
over panicking. Panicking should only happen due to bugs (programmer errors /
contract violations), which should be fixed during development phase. Handling
both cases in the same way makes it hard to spot real bugs.

let byte = Serial.read().unwrap_or_else(|| fail_safe());

fn fail_safe() -> ! {
    FATAL_LED.on();

    // turn off actuators, or
    // sound alarm, or
    // w/e makes sense for this application

    // halt system
    loop {
        interrupt::disable();
    }
}

I can reword this line as "Explicit panicking (e.g. unwrap) is used where
proper error handling is possible.", but the bottom line here is that explicit
panicking is a "code smell" and demands a closer inspection of the code.

Choose a reason for hiding this comment

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

IMO, jumping to a "fail-safe" state when unrecoverable I/O errors happen is preferable
over panicking. Panicking should only happen due to bugs (programmer errors /
contract violations), which should be fixed during development phase.

Sure a failsafe would be preferable but that may not always be available, also for some applications halting is a no-go and a reset is preferred instead. There're are other classes or errors which can happen for which they might not be a useful handling procedure like if there're pluggable components and those are not present but required for operation.

Everything here has a big dependency on the application and the hardware and IMHO there's neither a one fits all nor a good/bad classification possible. I'd rather encourage finding creative ways to avoid panicking rather than penalisation. Also instead of unwrapping an irrelevant Result I'd rather suggest using ok().

@japaric
Copy link
Member Author

japaric commented Feb 8, 2019

@rust-embedded/resources any other thoughts on merging this?

@japaric
Copy link
Member Author

japaric commented Feb 19, 2019

Let's land this since we need the "how to submit a project" text in the README for the newsletter.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 19, 2019

👎 Rejected by too few approved reviews

@adamgreig
Copy link
Member

I agree, let's get these in, we can change them as we gain experience.

bors r+

bors bot added a commit that referenced this pull request Feb 20, 2019
6: Add evaluation guidelines and instructions on how to submit a project r=adamgreig a=japaric

The guidelines include all I can think of at the moment. We'll probably come up
with more of them once start reviewing user submissions. And, maybe, in the
future these guidelines could become a document on "good practices for
structuring embedded applications".

Q: Is there any additional information we'll like to request from the submitter,
apart from the information in `data.yml` (see #5)? This information would be
provided in the description of the pull request. If not then we don't need a
pull request template as I suggested in #3.

r? @rust-embedded/resources

closes #3
closes #4

Co-authored-by: Jorge Aparicio <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 20, 2019

Build succeeded

@bors bors bot merged commit 2f188f9 into master Feb 20, 2019
@bors bors bot deleted the eval branch February 20, 2019 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a "grading rubric" Create an issue / PR template
3 participants