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

SPI Plugin - added APA102 #722

Merged
merged 49 commits into from Jun 3, 2015

Conversation

Projects
None yet
3 participants
@s-light
Contributor

s-light commented Apr 29, 2015

basic functionality is working. (mainly copied the P9813)
Tested with RPi and LED-Strip:
there is some strange behavior of the first led-
it continuously shines green.. but if i check with an osci the data seems correct...
additional sometimes there seems to be noise on the datalines (leds are flickering)
eventually this is caused by my level shifter / breadboard setup..
will have to check this if i get an alternative chip.

on the software site the 'EndFrame' of the protocol is not right. but its working for up to some leds (not tested) -
i want to fix this but i did not get how this can be modified with the current system (i need some help/info for the 'Checkout' function)

s-light added some commits Apr 26, 2015

added APA102 LED - UNTESTED
based on the P9813 code.
changes are not tested.
have to check how the latch_bytes work...
added APA102 LED - UNTESTED
based on the P9813 code.
changes are not tested.
have to check how the latch_bytes work...
added Personality 7
(forget to add the Personality switch case)
next test ;-)
learning and getting the thing to know..
fixed some test thing..
unused variable warning ....
Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
data = backend.GetData(0, &length);
const uint8_t EXPECTED2[] = { 0, 0, 0, 0,
0xff, 0x4e, 0x38, 0x22,
0xff, 0, 0, 0,

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

I think this is the wrong output. Sending a shorter frame shouldn't change what the values of the pixels outside the frame are.

@nomis52

nomis52 May 2, 2015

Member

I think this is the wrong output. Sending a shorter frame shouldn't change what the values of the pixels outside the frame are.

Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
// test with other StartAddress
// set StartAddress
output.SetStartAddress(3);
// values 1 & 2 should not be visibel in SPI data stream

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

typo: visible

@nomis52

nomis52 May 2, 2015

Member

typo: visible

Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
options.pixel_count = 2;
// setup SPIOutput
SPIOutput output(m_uid, &backend, options);
// set personality to 7= Individual APA102

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

Set personality to 8: Combined APA102

@nomis52

nomis52 May 2, 2015

Member

Set personality to 8: Combined APA102

Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
// test with other StartAddress
// set StartAddress
output.SetStartAddress(3);
// values 1 & 2 should not be visibel in SPI data stream

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

typo: visible

@nomis52

nomis52 May 2, 2015

Member

typo: visible

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
pixel_data[3] = buffer.Get(first_slot); // Get Red
// set all pixel to same value
for (unsigned int i = 0; i < m_pixel_count; i++) {

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

I'd do

for (unsigned int i = 1; i <= m_pixel_count; i++) {
...
}

@nomis52

nomis52 May 2, 2015

Member

I'd do

for (unsigned int i = 1; i <= m_pixel_count; i++) {
...
}

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
void SPIOutput::CombinedAPA102Control(const DmxBuffer &buffer) {
// for Protocol details see IndividualAPA102Control
const uint8_t latch_bytes = 3 * APA102_SPI_BYTES_PER_PIXEL;

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

I think this should be

const uint8_t latch_bits = m_pixel_count / 2;
const uint8_t latch_bytes = (latch_bits / 8) + (latch_bits % 8 ? 1: 0);

Same comment above, so I'd move this into a function:

CalculateAPA102LatchBytes(m_pixel_count);

@nomis52

nomis52 May 2, 2015

Member

I think this should be

const uint8_t latch_bits = m_pixel_count / 2;
const uint8_t latch_bytes = (latch_bits / 8) + (latch_bits % 8 ? 1: 0);

Same comment above, so I'd move this into a function:

CalculateAPA102LatchBytes(m_pixel_count);

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
}
// get data for entire string length
const unsigned int output_length = m_pixel_count * APA102_SPI_BYTES_PER_PIXEL;

This comment has been minimized.

@nomis52

nomis52 May 2, 2015

Member

This should be

const unsigned int output_length = (1 + m_pixel_count) * APA102_SPI_BYTES_PER_PIXEL;

@nomis52

nomis52 May 2, 2015

Member

This should be

const unsigned int output_length = (1 + m_pixel_count) * APA102_SPI_BYTES_PER_PIXEL;

fixed / incorporated suggestions
fixed some lint errors travis found
and incorporated fixes/suggestions from nomis52

todo: test in on hardware
@s-light

This comment has been minimized.

Show comment
Hide comment
@s-light

s-light May 4, 2015

Contributor

thanks nomis52 for your suggestions - i think i have understand most of them and incorporated them -
i will have a look at the thing at the end of the week and will also check with an real live hardware test... ;-)

Contributor

s-light commented May 4, 2015

thanks nomis52 for your suggestions - i think i have understand most of them and incorporated them -
i will have a look at the thing at the end of the week and will also check with an real live hardware test... ;-)

s-light added some commits May 4, 2015

next test
i don't really now what i am doing ;-)
added some debug output stuff
(hopefully OLA_INFO is the right way for this?!)
added APA102_START_FRAME_BYTES
and cleaned up all output handling to use the APA102_START_FRAME_BYTES
const.
fixed behaivor if for partial update
hopefully fixed a bug:
what should the Output of the Pixels be if only partyly data received?
i think they should not change.. so i had to fix this int eh
SPIOutput.cpp
but eventually this would also make sens to change for the P9813..
fixed LEDFrame start byte
LEDFram start byte must be set for all available pixels - regardless if
we have data for this or not..
fixed latchbytes calculation
and added a Test for this
dont know how to add the tests..
i have to check that later..
one possibility is to use a separat test for 16 and 17 pixel
@s-light

This comment has been minimized.

Show comment
Hide comment
@s-light

s-light May 6, 2015

Contributor

update:
i tried the code on the RaspberryPi and it is working :-)
as levelshifter i used an TI SN74HCT08 (like in the board linked from the OLA LED Pixels Tutorial)

i first tried to use the TXB0104 Bi-Directional Level Shifter on an Breakoutboard from Adafruit but this did not work - i think the RPI has problems to drive the chip..

my test setup:
p1540716_small
the signal with 1pixel (full protocol frame)
p1540750_1pixel_small
as 'speed' i tested 125KHz up to 10MHz - and it worked as fare as i could see this :-)
p1540753_125khz_small
p1540756_1mhz_small
p1540759_10mhz_small
(channel1 has some problems in higher frequency...)

Contributor

s-light commented May 6, 2015

update:
i tried the code on the RaspberryPi and it is working :-)
as levelshifter i used an TI SN74HCT08 (like in the board linked from the OLA LED Pixels Tutorial)

i first tried to use the TXB0104 Bi-Directional Level Shifter on an Breakoutboard from Adafruit but this did not work - i think the RPI has problems to drive the chip..

my test setup:
p1540716_small
the signal with 1pixel (full protocol frame)
p1540750_1pixel_small
as 'speed' i tested 125KHz up to 10MHz - and it worked as fare as i could see this :-)
p1540753_125khz_small
p1540756_1mhz_small
p1540759_10mhz_small
(channel1 has some problems in higher frequency...)

Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
// setup SPIOutput
SPIOutput output(m_uid, &backend, options);
// set personality
output.SetPersonality(thisTestPersonality);

This comment has been minimized.

@s-light

s-light May 6, 2015

Contributor

At this point i tried to add two tests to test the endFrame length.
but as fare as i found it is not possible to change the pixel_count after the initialization of the output.
and i did not know how to 'reinitialize' the output with new options..

@s-light

s-light May 6, 2015

Contributor

At this point i tried to add two tests to test the endFrame length.
but as fare as i found it is not possible to change the pixel_count after the initialization of the output.
and i did not know how to 'reinitialize' the output with new options..

This comment has been minimized.

@nomis52

nomis52 May 7, 2015

Member

There isn't a way to change the pixel_count of an Output. Just create a new one each time.

@nomis52

nomis52 May 7, 2015

Member

There isn't a way to change the pixel_count of an Output. Just create a new one each time.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
*/
uint8_t SPIOutput::CalculateAPA102LatchBytes(unsigned int m_pixel_count) {
// round up so that we get definitely more LatchBits as LEDs/2...
const uint8_t latch_bits = (m_pixel_count / 2) + (m_pixel_count % 2 ? 1: 0);

This comment has been minimized.

@nomis52

nomis52 May 7, 2015

Member

You can change the last bit to

  • (m_pixel_count % 2)
@nomis52

nomis52 May 7, 2015

Member

You can change the last bit to

  • (m_pixel_count % 2)

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

Can we actually just do a ceil please, given that's mathmatically what we want, then the compiler can optimise it if possible, and its clearer for future developers.

@peternewman

peternewman May 10, 2015

Member

Can we actually just do a ceil please, given that's mathmatically what we want, then the compiler can optimise it if possible, and its clearer for future developers.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// for part of it
const unsigned int output_length = APA102_START_FRAME_BYTES +
(m_pixel_count * APA102_SPI_BYTES_PER_PIXEL);
// OLA_DEBUG << "output_length " << static_cast<int>(output_length);

This comment has been minimized.

@peternewman

peternewman May 18, 2015

Member

Remove this or uncomment.

@peternewman

peternewman May 18, 2015

Member

Remove this or uncomment.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// set all pixel to same value
for (uint16_t i = 0; i < m_pixel_count; i++) {
uint16_t spi_offset = APA102_START_FRAME_BYTES +
(i * APA102_SPI_BYTES_PER_PIXEL);

This comment has been minimized.

@peternewman

peternewman May 18, 2015

Member

Can you indent this four from uint16_t if it won't all fit on one line.

@peternewman

peternewman May 18, 2015

Member

Can you indent this four from uint16_t if it won't all fit on one line.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
/**
* Calculate Latch Bytes for APA102:
* minimal use half the pixel count bits

This comment has been minimized.

@peternewman

peternewman May 18, 2015

Member

Nit: "Use at least half the pixel count bits"

@peternewman

peternewman May 18, 2015

Member

Nit: "Use at least half the pixel count bits"

Show outdated Hide outdated plugins/spi/SPIOutputTest.cpp
@@ -235,7 +239,7 @@ void SPIOutputTest::testIndividualLPD8806Control() {
buffer.SetFromString("255,128,0,10,20,30");
output.WriteDMX(buffer);
data = backend.GetData(0, &length);
const uint8_t EXPECTED1[] = { 0xc0, 0xff, 0x80, 0x8a, 0x85, 0x8f, 0 };
const uint8_t EXPECTED1[] = { 0xc0, 0xFF, 0x80, 0x8a, 0x85, 0x8f, 0 };

This comment has been minimized.

@peternewman

peternewman May 18, 2015

Member

Sorry to nit pick, if we're doing that, can we capitalise the other hex in the tests too please.

@peternewman

peternewman May 18, 2015

Member

Sorry to nit pick, if we're doing that, can we capitalise the other hex in the tests too please.

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman May 18, 2015

Member

LGTM aside from comments.

Member

peternewman commented May 18, 2015

LGTM aside from comments.

@s-light

This comment has been minimized.

Show comment
Hide comment
@s-light

s-light May 25, 2015

Contributor

thanks for your comments - i hope to get to it tomorrow - last week my computer was down.. and i had to resetup all the things.... and moved from windows to kubuntu - that was a big thing to do.. ;-)

Contributor

s-light commented May 25, 2015

thanks for your comments - i hope to get to it tomorrow - last week my computer was down.. and i had to resetup all the things.... and moved from windows to kubuntu - that was a big thing to do.. ;-)

s-light added some commits May 28, 2015

Merge branch 'master' of https://github.com/OpenLightingProject/ola i…
…nto SPI_APA102

Conflicts:
	plugins/spi/SPIOutput.cpp
Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// some detailed information on the protocol:
// https://cpldcpu.wordpress.com/2014/11/30/understanding-the-apa102-superled/
// Data-Struct
// StartFrame: 4Byte = 32Bits zeros (APA102_START_FRAME_BYTES)

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Really minor nits, but 4 bytes and 32 bits

@peternewman

peternewman May 28, 2015

Member

Really minor nits, but 4 bytes and 32 bits

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// calculate DMX-start-address
const unsigned int first_slot = m_start_address - 1; // 0 offset
// only do something if at least 1pixel can be updated..

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

1 pixel

@peternewman
Show outdated Hide outdated plugins/spi/SPIOutput.cpp
const unsigned int first_slot = m_start_address - 1; // 0 offset
// only do something if at least 1pixel can be updated..
if (buffer.Size() - first_slot < APA102_SLOTS_PER_PIXEL) {

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Can you add brackets round the negation please.

@peternewman

peternewman May 28, 2015

Member

Can you add brackets round the negation please.

This comment has been minimized.

@nomis52

nomis52 May 28, 2015

Member

No, please don't, they aren't needed here and just clutter the code.

@nomis52

nomis52 May 28, 2015

Member

No, please don't, they aren't needed here and just clutter the code.

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Okay.

@peternewman
Show outdated Hide outdated plugins/spi/SPIOutput.cpp
(i * APA102_SPI_BYTES_PER_PIXEL);
// set pixel data
// first Byte contains:
// 3bit start mark (111) + 5bit GlobalBrightnes

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Minor nit 3 bits, 5 bits

@peternewman

peternewman May 28, 2015

Member

Minor nit 3 bits, 5 bits

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

SPaG Brightness, but perhaps change it to global brightness

@peternewman

peternewman May 28, 2015

Member

SPaG Brightness, but perhaps change it to global brightness

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// set pixel data
// first Byte contains:
// 3bit start mark (111) + 5bit GlobalBrightnes
// set GlobalBrightnes fixed to 31 --> that reduces flickering

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

SPaG Brightness, or global brightness

@peternewman

peternewman May 28, 2015

Member

SPaG Brightness, or global brightness

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// get data for entire string length
const uint16_t output_length = (m_pixel_count + 1) *
APA102_SPI_BYTES_PER_PIXEL;

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Can you line this up with the start of the (, or four after const

@peternewman

peternewman May 28, 2015

Member

Can you line this up with the start of the (, or four after const

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
const uint16_t output_length = (m_pixel_count + 1) *
APA102_SPI_BYTES_PER_PIXEL;
uint8_t *output = m_backend->Checkout(m_output_number, output_length,
CalculateAPA102LatchBytes(m_pixel_count));

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Indent to the end of (, or four after uint8_t

@peternewman

peternewman May 28, 2015

Member

Indent to the end of (, or four after uint8_t

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// skip spi_offset + 0 (is already set)
output[spi_offset + 1] = buffer.Get(offset + 2); // blue
output[spi_offset + 2] = buffer.Get(offset + 1); // green
output[spi_offset + 3] = buffer.Get(offset + 0); // red

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

Either remove the + 0 here, or add it at line 587.

@peternewman

peternewman May 28, 2015

Member

Either remove the + 0 here, or add it at line 587.

This comment has been minimized.

@peternewman

peternewman May 29, 2015

Member

Sorted on line 587.

@peternewman

peternewman May 29, 2015

Member

Sorted on line 587.

This comment has been minimized.

@peternewman

peternewman Jun 2, 2015

Member

Given @nomis52 comments, can we remove the + 0 here please.

@peternewman

peternewman Jun 2, 2015

Member

Given @nomis52 comments, can we remove the + 0 here please.

s-light added some commits May 28, 2015

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
}
// get data for entire string length
const uint16_t output_length = (m_pixel_count + 1) *

This comment has been minimized.

@nomis52

nomis52 Jun 1, 2015

Member

Should this be the same as line 540? It's confusing to express it two different ways. I'd settle on

unsigned int spi_offset = APA102_START_FRAME_BYTES + i * APA102_SPI_BYTES_PER_PIXEL;

@nomis52

nomis52 Jun 1, 2015

Member

Should this be the same as line 540? It's confusing to express it two different ways. I'd settle on

unsigned int spi_offset = APA102_START_FRAME_BYTES + i * APA102_SPI_BYTES_PER_PIXEL;

This comment has been minimized.

@s-light

s-light Jun 1, 2015

Contributor

i will take a look at what was my intension tomorrow..

@s-light

s-light Jun 1, 2015

Contributor

i will take a look at what was my intension tomorrow..

This comment has been minimized.

@s-light

s-light Jun 2, 2015

Contributor

have fixed this - that was a copy and paste relict

@s-light

s-light Jun 2, 2015

Contributor

have fixed this - that was a copy and paste relict

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
const uint16_t output_length = (m_pixel_count + 1) *
APA102_SPI_BYTES_PER_PIXEL;
uint8_t *output = m_backend->Checkout(m_output_number, output_length,
CalculateAPA102LatchBytes(m_pixel_count));

This comment has been minimized.

@nomis52

nomis52 Jun 1, 2015

Member

Please fix the indentation here.

@nomis52

nomis52 Jun 1, 2015

Member

Please fix the indentation here.

This comment has been minimized.

@s-light

s-light Jun 1, 2015

Contributor

sorry - i changed this after the comment from peternewman #722 (diff)
how should i line it up correctly? (its to long for one line)

@s-light

s-light Jun 1, 2015

Contributor

sorry - i changed this after the comment from peternewman #722 (diff)
how should i line it up correctly? (its to long for one line)

This comment has been minimized.

@nomis52

nomis52 Jun 2, 2015

Member

I'd do:

uint8_t *output = m_backend->Checkout(
m_output_number, output_length,
CalculateAPA102LatchBytes(m_pixel_count));

That's a 4 space indent for the 2nd and 3rd lines.

@nomis52

nomis52 Jun 2, 2015

Member

I'd do:

uint8_t *output = m_backend->Checkout(
m_output_number, output_length,
CalculateAPA102LatchBytes(m_pixel_count));

That's a 4 space indent for the 2nd and 3rd lines.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
pixel_data[1] = buffer.Get(first_slot + 2); // Get Blue
pixel_data[2] = buffer.Get(first_slot + 1); // Get Green
pixel_data[3] = buffer.Get(first_slot + 0); // Get Red

This comment has been minimized.

@nomis52

nomis52 Jun 1, 2015

Member

Please initialize the first 4 bytes here rather than relying on the default values.

@nomis52

nomis52 Jun 1, 2015

Member

Please initialize the first 4 bytes here rather than relying on the default values.

This comment has been minimized.

@s-light

s-light Jun 1, 2015

Contributor

so you mean to write:

// create Pixel Data
uint8_t pixel_data[APA102_SPI_BYTES_PER_PIXEL];
pixel_data[0] = 0xFF;
pixel_data[1] = 0x00;
pixel_data[2] = 0x00;
pixel_data[3] = 0x00;
pixel_data[1] = buffer.Get(first_slot + 2);  // Get Blue
....

?

@s-light

s-light Jun 1, 2015

Contributor

so you mean to write:

// create Pixel Data
uint8_t pixel_data[APA102_SPI_BYTES_PER_PIXEL];
pixel_data[0] = 0xFF;
pixel_data[1] = 0x00;
pixel_data[2] = 0x00;
pixel_data[3] = 0x00;
pixel_data[1] = buffer.Get(first_slot + 2);  // Get Blue
....

?

This comment has been minimized.

@nomis52

nomis52 Jun 2, 2015

Member

I was referring to the first 4 bytes of output. Do something like

memset(output, 0, APA102_START_FRAME_BYTES);

@nomis52

nomis52 Jun 2, 2015

Member

I was referring to the first 4 bytes of output. Do something like

memset(output, 0, APA102_START_FRAME_BYTES);

This comment has been minimized.

@s-light

s-light Jun 2, 2015

Contributor

ok - now i got it :-)

@s-light

s-light Jun 2, 2015

Contributor

ok - now i got it :-)

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
// 3 bits start mark (111) + 5 bits global brightness
// set global brightness fixed to 31 --> that reduces flickering
// that can be written as 0xE0 & 0x1F
output[spi_offset + 0] = 0xFF;

This comment has been minimized.

@nomis52

nomis52 Jun 1, 2015

Member

remove the + 0

@nomis52

nomis52 Jun 1, 2015

Member

remove the + 0

if (!output) {
return;
}

This comment has been minimized.

@nomis52

nomis52 Jun 1, 2015

Member

Please initialize the first 4 bytes here as well rather than relying on the defaults.

@nomis52

nomis52 Jun 1, 2015

Member

Please initialize the first 4 bytes here as well rather than relying on the defaults.

This comment has been minimized.

@s-light

s-light Jun 2, 2015

Contributor

done

@s-light

s-light Jun 2, 2015

Contributor

done

@nomis52

This comment has been minimized.

Show comment
Hide comment
@nomis52

nomis52 Jun 1, 2015

Member

Just minor comments. The only one I feel strongly about is initializing the first 4 bytes. Otherwise it looks like we forgot to do it and it's not clear to people later what values they are supposed to take.

Member

nomis52 commented Jun 1, 2015

Just minor comments. The only one I feel strongly about is initializing the first 4 bytes. Otherwise it looks like we forgot to do it and it's not clear to people later what values they are supposed to take.

@s-light

This comment has been minimized.

Show comment
Hide comment
@s-light

s-light Jun 1, 2015

Contributor

thanks for the review nomis52,
i will have fix it tomorrow.

Contributor

s-light commented Jun 1, 2015

thanks for the review nomis52,
i will have fix it tomorrow.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
pixel_data[0] = 0xFF;
pixel_data[1] = buffer.Get(first_slot + 2); // Get Blue
pixel_data[2] = buffer.Get(first_slot + 1); // Get Green
pixel_data[3] = buffer.Get(first_slot + 0); // Get Red

This comment has been minimized.

@peternewman

peternewman Jun 2, 2015

Member

And here too.

@peternewman

peternewman Jun 2, 2015

Member

And here too.

Show outdated Hide outdated plugins/spi/SPIOutput.cpp
void SPIOutput::CombinedAPA102Control(const DmxBuffer &buffer) {
// for Protocol details see IndividualAPA102Control

This comment has been minimized.

@peternewman

peternewman Jun 2, 2015

Member

Remove all the extra whitespace.

@peternewman

peternewman Jun 2, 2015

Member

Remove all the extra whitespace.

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Jun 2, 2015

I'd be tempted to split these two onto their own lines, but that's sort of a personal preference @nomis52 may feel differently.

I'd be tempted to split these two onto their own lines, but that's sort of a personal preference @nomis52 may feel differently.

This comment has been minimized.

Show comment
Hide comment
@nomis52

nomis52 Jun 2, 2015

Either way is fine with me. I tend to prefer using less vertical space.

nomis52 replied Jun 2, 2015

Either way is fine with me. I tend to prefer using less vertical space.

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Jun 2, 2015

Remove this blank line.

Remove this blank line.

@s-light

This comment has been minimized.

Show comment
Hide comment
@s-light

s-light Jun 2, 2015

Contributor

sorry for the extra whitespace (was for aligning to sync the writings...)
hopefully i have found it all.

Contributor

s-light commented Jun 2, 2015

sorry for the extra whitespace (was for aligning to sync the writings...)
hopefully i have found it all.

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Jun 2, 2015

Member

Thanks @s-light can you just remove those extra + 0's, then I think we're good to go if @nomis52 is happy.

Member

peternewman commented Jun 2, 2015

Thanks @s-light can you just remove those extra + 0's, then I think we're good to go if @nomis52 is happy.

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Jun 3, 2015

Member

I'm happy now, over to @nomis52 for a final review.

Member

peternewman commented Jun 3, 2015

I'm happy now, over to @nomis52 for a final review.

@nomis52

This comment has been minimized.

Show comment
Hide comment
@nomis52

nomis52 Jun 3, 2015

Member

LGTM

Member

nomis52 commented Jun 3, 2015

LGTM

nomis52 added a commit that referenced this pull request Jun 3, 2015

@nomis52 nomis52 merged commit 910784e into OpenLightingProject:master Jun 3, 2015

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@s-light s-light deleted the s-light:SPI_APA102 branch Jun 4, 2015

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