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

More int type fixes and tests for drivers #80

Merged
merged 6 commits into from Jul 8, 2014

Conversation

Projects
None yet
4 participants
@errordeveloper
Member

errordeveloper commented Jul 2, 2014

Just pulled in nightly and got a whole bunch of errors like this:

error: cannot determine a type for this expression: cannot determine the type of this integer; add a suffix to specify the type explicitly

Not sure, may be this just a follow-up on rust-lang/rust#6023?

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

Not sure how that is "making the language more simple", but whatever. Just one comment above

@@ -121,7 +121,7 @@ impl<'a, S: SPI, T: Timer, P: GPIO> C12332<'a, S, T, P> {
let index = x + (y/8) * 128;
if color == 0 {
self.videobuf[index as uint].set(
self.videobuf[index as uint].get() & !(1 << (y%8) as uint) as u8);
self.videobuf[index as uint].get() & !(1i32 << (y%8i32) as uint) as u8);

This comment has been minimized.

@farcaller

farcaller Jul 2, 2014

Member

Why is this signed int? it's getting downcasted to u8, so using u8 should be fine.

This comment has been minimized.

@errordeveloper

errordeveloper Jul 2, 2014

Member

You are right!

This comment has been minimized.

@errordeveloper

errordeveloper Jul 2, 2014

Member

Ok, that gives me another error:

/Users/ilya/Code/Work/rust/zinc/src/drivers/lcd/c12332.rs:124:58: 124:61 error: mismatched types: expected `i32` but found `i8` (expected i32 but found i8)
/Users/ilya/Code/Work/rust/zinc/src/drivers/lcd/c12332.rs:124         self.videobuf[index as uint].get() & !(1i8 << (y%8i8) as uint) as u8);

This comment has been minimized.

@farcaller

farcaller Jul 2, 2014

Member

Ah. so, y being i32 is kind of a bug, as you cannot really address any pixel < 0. If you have some time to work on this, can you please update function definition to read pub fn set_pixel(&self, x: u32, y: u32, color: u16) ? Then the addressing should be something along the lines of (1u8 << (y % 8u32) as uint) as u8

This comment has been minimized.

@errordeveloper

errordeveloper Jul 2, 2014

Member

yes, that's what I was thinking, but wasn't sure...

This comment has been minimized.

@errordeveloper

errordeveloper Jul 2, 2014

Member

And is this a bug as well?

src/drivers/lcd/mod.rs:  fn pixel(&self, x: i32, y: i32, color: u16);

This comment has been minimized.

@farcaller

farcaller Jul 2, 2014

Member

Yes, that one as well.

This comment has been minimized.

@errordeveloper

errordeveloper Jul 2, 2014

Member

Ok, will fix it.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

I won't be surprised it will go away. May be we need refactor those kind of expressions somehow?

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

Most of that is bits manipulation. It would be nice to have some macro to do that in a better (and more safe way).

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

It certainly needs some nice macros.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

Looks a little cleaner now 😄

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

Yeah... The warning, wasn't quite sure what's the fix...
On 2 Jul 2014 18:32, "Vladimir Pouzanov" notifications@github.com wrote:

A bit mode cleaning please:
https://travis-ci.org/errordeveloper/zinc/jobs/28985715#L103

Reply to this email directly or view it on GitHub
#80 (comment).

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

In c12332 — it's safe to remove the check for < 0.

In lcd/mod.rs — you actually broke the algorithm, all the variables other than x0/y0 can be less than zero.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

So 35440a6 fixes C12332. And for the broken algorithm I'm thinking of fixing it with if di < 0i as u32, which seem about right to me.

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

No, di must be i32, as di = dy_x2 - dx might be less than zero.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

Well, if they are unsigned, they will never be less then zero, it will
simply overflow, isn't it?
On 2 Jul 2014 20:31, "Vladimir Pouzanov" notifications@github.com wrote:

No, di must be i32, as di = dy_x2 - dx might be less than zero.

Reply to this email directly or view it on GitHub
#80 (comment).

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

So in terms of geometry, what does the zero represent here? Is it more like
the centre of a line (-∞:0:+∞) or beginning of a line [0:+∞)?

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

@farcaller I'm thinking that we should have test for this, generate a bitmap and compare it to a static bitmap, what do you think?

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 2, 2014

Ok. I can run the test on actual hw this weekend, but you can go on and make an unit test.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 2, 2014

Yeah, I'd love to! Might need a little help, if you don't mind... Will ping
you tomorrow ;)
On 2 Jul 2014 21:57, "Vladimir Pouzanov" notifications@github.com wrote:

Ok. I can run the test on actual hw this weekend, but you can go on and
make an unit test.

Reply to this email directly or view it on GitHub
#80 (comment).

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 4, 2014

Will re-open when unit tests are done.

@errordeveloper errordeveloper reopened this Jul 6, 2014

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 6, 2014

@farcaller this is on top of #89.

@farcaller farcaller changed the title from More int type fixes to More int type fixes and tests for drivers Jul 6, 2014

Rakefile Outdated
source: 'main.rs'.in_source,
deps: [:core_crate],
produce: 'zinc_test'.in_build,
recompile_on: [:triple, :platform, :features],

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Do not recompile on :triple, host triple isn't going to change.

pub mod test {
use core::cell::RefCell;
use drivers::chario::CharIO;
//use core::option::{Option, Some, None};

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Clean this up

#[cfg(test)]
pub mod test {
use core::cell::RefCell;
use drivers::chario::CharIO;

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

One empty line between system and local use

use drivers::chario::CharIO;
//use core::option::{Option, Some, None};
pub struct TestCharIOData { last_char: char, putc_calls: uint }

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Expand this into several lines

pub struct TestCharIOData { last_char: char, putc_calls: uint }
pub struct TestCharIO {
//data: RefCell<Option<TestCharIOData>>

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Clean up

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Also, given it's the only field, pub struct TestCharIO (RefCell<Option<TestCharIOData>>) (tuple struct) is preferred

pub fn new() -> TestCharIO {
TestCharIO {
data: RefCell::new(TestCharIOData {
last_char: '\0',

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

Fix identation

fn get_and_reset_putc_calls(&self) -> uint {
let current = self.data.borrow().putc_calls;
self.data.borrow_mut().putc_calls = 0;
return current;

This comment has been minimized.

@farcaller

farcaller Jul 6, 2014

Member

just current

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 6, 2014

I think you can do with a much smaller buffer, like 5x5 pixels, this way you can make a helper test case macro that makes a call and verifies it by the "picture". Bonus points if you define the template with something like

let match = "
.....
.XXXX
.X..X
.XXXX
....."
@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 6, 2014

@farcaller regarding the buffer, I though might as well print a letter using putc, so 16x16 seemed like good enough...

@errordeveloper errordeveloper referenced this pull request Jul 7, 2014

Closed

Add test to CharIO #89

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 7, 2014

Travis might have hit a rustc bug, but I'd rather rebase this on #92 first. I'm also thinking to squash this down to a couple of commits. From 15 to 3 or 5, perhaps...

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 7, 2014

Does anyone else want to review this or we can merge?

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 7, 2014

Please remove this test: test dummy_test ... ok

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 7, 2014

Removed. Should I squash or it's okay?

io.line(0, 0, 15, 15, 1);
// TODO: investigate why this hangs the test run in `__psynch_cvwait()`

This comment has been minimized.

@farcaller

farcaller Jul 7, 2014

Member

Please 'own' the todo: // TODO(errordeveloper): ...

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 7, 2014

Yes, you can squash it a bit (reasonably).

errordeveloper added some commits Jul 5, 2014

Refactor LCD mod -
- pixel coordinates are always unsigned
- use `for ... in range(...)` instead of `while ...`
fn line(&self, x0_b: u32, y0_b: u32, x1: u32, y1: u32, color: u16) {
let mut x0: u32 = x0_b;
let mut y0: u32 = y0_b;
let mut dx: u32;

This comment has been minimized.

@bharrisau

bharrisau Jul 7, 2014

Contributor

Do these dx want to be i32?

This comment has been minimized.

@bharrisau

bharrisau Jul 7, 2014

Contributor

dx, dy, dx_sym and dy_sym at the minimum should be signed. This should fix your hanging test.

This comment has been minimized.

@bharrisau

bharrisau Jul 7, 2014

Contributor

And it might be nicer if you get rid of the 'mut' and just rebind with another let statement. di, x0 and y0 should be mut.

This comment has been minimized.

@farcaller

farcaller Jul 7, 2014

Member

Yes, those must me signed, as dx = x1-x0 may be less than zero as we discussed above. @errordeveloper, please fix and re-enable the 15,15 -> 0,0 test, it should work.

@bharrisau, thanks for catching that.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 7, 2014

Tests looks great. Thanks for this.

Refactor LCD even further -
- fix hanging test by fixing signed/unsigned types
- get rid of C copypastaz and not needed mut vars
- more refactoring of while loops
@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 8, 2014

We don't get any notifications when you update things - so you need to post here if you are ready for review.

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 8, 2014

Well, I do have notifications for stale "ready for review" PRs, but the script is broken :)

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 8, 2014

A simple r? should be fine. I don't think non-contributors can update
labels on issues.

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 8, 2014

oh well. yes, then a simple 'r?' is fine, somehow I missed the permissions bit.

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jul 8, 2014

So is this good to merge now?

@farcaller

This comment has been minimized.

farcaller commented on 3b083b9 Jul 8, 2014

r+

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 8, 2014

Yup, thanks for your work.

@hacknbot

This comment has been minimized.

Contributor

hacknbot commented on 3b083b9 Jul 8, 2014

saw approval from farcaller
at errordeveloper@3b083b9

This comment has been minimized.

Contributor

hacknbot replied Jul 8, 2014

merging errordeveloper/zinc/more_int_fixes = 3b083b9 into auto

This comment has been minimized.

Contributor

hacknbot replied Jul 8, 2014

errordeveloper/zinc/more_int_fixes = 3b083b9 merged ok, testing candidate = c008f64

This comment has been minimized.

Contributor

hacknbot replied Jul 8, 2014

This comment has been minimized.

Contributor

hacknbot replied Jul 8, 2014

fast-forwarding master to auto = c008f64

hacknbot added a commit that referenced this pull request Jul 8, 2014

Merge pull request #80 from errordeveloper/more_int_fixes
More int type fixes and tests for drivers

Reviewed-by: farcaller

@hacknbot hacknbot merged commit 3b083b9 into hackndev:master Jul 8, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default all tests passed
Details

@errordeveloper errordeveloper deleted the errordeveloper:more_int_fixes branch Jul 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment