Add a small LED interface and demo app.

Add a small LED interface and demo app.

BUG=

[ricow1, 2016-02-09 09:06:07]

ricow@google.com changed reviewers:
+ ricow@google.com

[ricow1, 2016-02-09 09:06:08]

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart
File dart/examples/led/led.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:9: int count = 0;
remove unused variable

[Søren Gjesse, 2016-02-09 09:16:01]

sgjesse@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2016-02-09 09:16:01]

I just landed a new GPIO API for the "non lk" part,
https://github.com/dartino/sdk/commit/36139cdc5e55fc00fe7925fee49f00b5e609c04b.
Maybe we should think about consolidating?

https://codereview.chromium.org/1684433003/diff/1/app/dartino/leds.c
File app/dartino/leds.c (right):

https://codereview.chromium.org/1684433003/diff/1/app/dartino/leds.c#newcode2
app/dartino/leds.c:2: // Copyright (c) 2015, the Dartino project authors. Please
see the AUTHORS file
2016

https://codereview.chromium.org/1684433003/diff/1/app/dartino/leds.c#newcode33
app/dartino/leds.c:33: #else
Can't we just say #error here?

https://codereview.chromium.org/1684433003/diff/1/app/dartino/main.c
File app/dartino/main.c (right):

https://codereview.chromium.org/1684433003/diff/1/app/dartino/main.c#newcode83
app/dartino/main.c:83: DARTINO_EXPORT_TABLE_ENTRY("led_toggle",
change_led_status)
Why is is called led_toggle when exported? Wouldn't set_led_state be more
precise (maybe use that name for the actual C function as well.

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart
File dart/examples/led/led.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:27: new Timer.periodic(const Duration(milliseconds:
50), makeCallback(LED0));
If you add toggle to LED (and make state both a getter and setter), then each of
these lines could be:

new Timer.periodic(const Duration(milliseconds: 100), () => LED1.toggle());

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart
File dart/pkg/lk/lib/leds.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart#n...
dart/pkg/lk/lib/leds.dart:14: setState(bool value) {
Just make this a setter.

And please add a getter for state and a toggle method (see comments in previous
file).

[lukechurch, 2016-02-09 09:20:01]

lukechurch@google.com changed reviewers:
+ lukechurch@google.com

[lukechurch, 2016-02-09 09:20:01]

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart
File dart/examples/led/led.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:31: }
Consider adding an explanatory comment as to why the program doesn't immediately
exit here.

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart
File dart/pkg/lk/lib/leds.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart#n...
dart/pkg/lk/lib/leds.dart:12: const LED._internal(this._id);
Consider an explanatory comment as to the pattern that you're using here, e.g.
the use of privacy to stop people accidentally creating extra LEDs that have no
corresponding hardward

[herhut, 2016-02-09 10:10:15]

PTAL

https://codereview.chromium.org/1684433003/diff/1/app/dartino/leds.c
File app/dartino/leds.c (right):

https://codereview.chromium.org/1684433003/diff/1/app/dartino/leds.c#newcode33
app/dartino/leds.c:33: #else
On 2016/02/09 09:16:01, Søren Gjesse wrote:
> Can't we just say #error here?

No, cause then it won't build on qemu, which is used for general testing of
other stuff. We do not have (and probably don't need) LED emulation on QEmu.

I have changed it into a lookup function for the GPIO mapping, which on qemu
always returns -1 (meaning not present).

https://codereview.chromium.org/1684433003/diff/1/app/dartino/main.c
File app/dartino/main.c (right):

https://codereview.chromium.org/1684433003/diff/1/app/dartino/main.c#newcode83
app/dartino/main.c:83: DARTINO_EXPORT_TABLE_ENTRY("led_toggle",
change_led_status)
On 2016/02/09 09:16:01, Søren Gjesse wrote:
> Why is is called led_toggle when exported? Wouldn't set_led_state be more
> precise (maybe use that name for the actual C function as well.

Because it is so long and these turn into strings :)

I've changed it...

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart
File dart/examples/led/led.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:9: int count = 0;
On 2016/02/09 09:06:08, ricow1 wrote:
> remove unused variable

Thanks!

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:27: new Timer.periodic(const Duration(milliseconds:
50), makeCallback(LED0));
On 2016/02/09 09:16:01, Søren Gjesse wrote:
> If you add toggle to LED (and make state both a getter and setter), then each
of
> these lines could be:
> 
> new Timer.periodic(const Duration(milliseconds: 100), () => LED1.toggle());

Done.

https://codereview.chromium.org/1684433003/diff/1/dart/examples/led/led.dart#...
dart/examples/led/led.dart:31: }
On 2016/02/09 09:20:01, lukechurch wrote:
> Consider adding an explanatory comment as to why the program doesn't
immediately
> exit here.

Done.

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart
File dart/pkg/lk/lib/leds.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart#n...
dart/pkg/lk/lib/leds.dart:12: const LED._internal(this._id);
On 2016/02/09 09:20:01, lukechurch wrote:
> Consider an explanatory comment as to the pattern that you're using here, e.g.
> the use of privacy to stop people accidentally creating extra LEDs that have
no
> corresponding hardward

I think this is a very common pattern and we should not describe it in every
system library where it is used.

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart#n...
dart/pkg/lk/lib/leds.dart:14: setState(bool value) {
On 2016/02/09 09:16:01, Søren Gjesse wrote:
> Just make this a setter.
> 
> And please add a getter for state and a toggle method (see comments in
previous
> file).

Done.

[lukechurch, 2016-02-09 10:13:54]

Thanks! LGTM

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart
File dart/pkg/lk/lib/leds.dart (right):

https://codereview.chromium.org/1684433003/diff/1/dart/pkg/lk/lib/leds.dart#n...
dart/pkg/lk/lib/leds.dart:12: const LED._internal(this._id);
On 2016/02/09 10:10:15, herhut wrote:
> On 2016/02/09 09:20:01, lukechurch wrote:
> > Consider an explanatory comment as to the pattern that you're using here,
e.g.
> > the use of privacy to stop people accidentally creating extra LEDs that have
> no
> > corresponding hardward
> 
> I think this is a very common pattern and we should not describe it in every
> system library where it is used. 

I agree. We should describe it once, in the place where people look before
writing their first hardware wrapper library in Dartino. If this isn't expected
to be that place, we can leave it out.

[Søren Gjesse, 2016-02-09 10:40:31]

lgtm

https://codereview.chromium.org/1684433003/diff/60001/dart/pkg/lk/lib/leds.dart
File dart/pkg/lk/lib/leds.dart (right):

https://codereview.chromium.org/1684433003/diff/60001/dart/pkg/lk/lib/leds.da...
dart/pkg/lk/lib/leds.dart:26: toggle() => state = ! state;
Remove space after !.