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
Commits
Jump to file or symbol
Failed to load files and symbols.
+83 −6
Diff settings

Always

Just for now

Viewing a subset of changes. View all

fixed latchbytes calculation

and added a Test for this
  • Loading branch information...
s-light committed May 6, 2015
commit e05f6f66ecf0d80448a3c8caf32c32dd46d34f1b
@@ -616,13 +616,15 @@ void SPIOutput::CombinedAPA102Control(const DmxBuffer &buffer) {
* 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"

* round up to next full byte count.
* datasheet says endframe should consist of 4 bytes -
* but thats only valid for up to 64 pixels/leds.
* but thats only valid for up to 64 pixels/leds. (4Byte*8Bit*2=64)
*
* the function is valid up to 4080 pixels. (255*8*2)
* ( otherwise the return type must be changed to uint16_t)
*/
uint8_t SPIOutput::CalculateAPA102LatchBytes(unsigned int m_pixel_count) {

This comment has been minimized.

@nomis52

nomis52 May 7, 2015

Member

m_pixel_count shadows the member variable with the same name. Call this pixel_count and make the method static.

@nomis52

nomis52 May 7, 2015

Member

m_pixel_count shadows the member variable with the same name. Call this pixel_count and make the method static.

const uint8_t latch_bits = m_pixel_count / 2;
// 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.

// same for latch_bytes - if latch_bits is odd-numbered add on more byte.
const uint8_t latch_bytes = (latch_bits / 8) + (latch_bits % 8 ? 1: 0);

This comment has been minimized.

@nomis52

nomis52 May 7, 2015

Member

You can just return on this line and save the intermediate variable.

@nomis52

nomis52 May 7, 2015

Member

You can just return on this line and save the intermediate variable.

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

Ceil again. Also once we've converted it, we can combine them both into one function:
return ceil(ceil(m_pixel_count/2)/8)

@peternewman

peternewman May 10, 2015

Member

Ceil again. Also once we've converted it, we can combine them both into one function:
return ceil(ceil(m_pixel_count/2)/8)

return latch_bytes;
}

This comment has been minimized.

@s-light

s-light May 13, 2015

Contributor

@peternewman i tried to use ceil but than the tests failed..
eventually i would have to cast one of the operands to float so that there is a float as result that can be 'ceiled' ?
but does this makes it clearer?

@peternewman & @nomis52 if you wish i will join the three lines - but i thought that with the separate lines and temporary variables it is clearer to read what the function does.. (and the compiler will optimize it away i think..)

@s-light

s-light May 13, 2015

Contributor

@peternewman i tried to use ceil but than the tests failed..
eventually i would have to cast one of the operands to float so that there is a float as result that can be 'ceiled' ?
but does this makes it clearer?

@peternewman & @nomis52 if you wish i will join the three lines - but i thought that with the separate lines and temporary variables it is clearer to read what the function does.. (and the compiler will optimize it away i think..)

This comment has been minimized.

@peternewman

peternewman May 13, 2015

Member

Regarding ceil, I'd be tempted to do the cast but I don't know what @nomis52 thinks, although is it not as simple as pixel_count / 2.0? Even with the cast it still feels clearer and easier to read to me; I know exactly what a ceil does when I see it, rather than having to look at your code and go through the various cases to understand that is what it's actually doing. Plus it removes the chance of a future tweak breaking that intention.

I'm not really a fan of temporary variables that are only used once, but that's just my personal preference.

@peternewman

peternewman May 13, 2015

Member

Regarding ceil, I'd be tempted to do the cast but I don't know what @nomis52 thinks, although is it not as simple as pixel_count / 2.0? Even with the cast it still feels clearer and easier to read to me; I know exactly what a ceil does when I see it, rather than having to look at your code and go through the various cases to understand that is what it's actually doing. Plus it removes the chance of a future tweak breaking that intention.

I'm not really a fan of temporary variables that are only used once, but that's just my personal preference.

This comment has been minimized.

@nomis52

nomis52 May 14, 2015

Member

Avoid using floating point ops, not all CPUs have an FPU. We can reduce the code to:

uint8_t latch_bits = (pixel_count + 1) / 2;
uint8_t latch_bytes = (latch_bits + 7) / 8;

I'm fine leaving the temporary in place.

@nomis52

nomis52 May 14, 2015

Member

Avoid using floating point ops, not all CPUs have an FPU. We can reduce the code to:

uint8_t latch_bits = (pixel_count + 1) / 2;
uint8_t latch_bytes = (latch_bits + 7) / 8;

I'm fine leaving the temporary in place.

@@ -430,15 +430,17 @@ void SPIOutputTest::testCombinedP9813Control() {
* Test DMX writes in the individual APA102 mode.
*/
void SPIOutputTest::testIndividualAPA102Control() {
// personality 7= Individual APA102
const uint16_t thisTestPersonality = 7;

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

Style nit, can we have this_test_personality please.

@peternewman

peternewman May 10, 2015

Member

Style nit, can we have this_test_personality please.

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

Or just stick it straight into the SetPersonality call.

@peternewman

peternewman May 10, 2015

Member

Or just stick it straight into the SetPersonality call.

// setup Backend
FakeSPIBackend backend(2);
SPIOutput::Options options(0, "Test SPI Device");
// setup pixel_count to 2 (enough to test all cases)
options.pixel_count = 2;
// setup SPIOutput
SPIOutput output(m_uid, &backend, options);
// set personality to 7= Individual APA102
output.SetPersonality(7);
// set personality
output.SetPersonality(thisTestPersonality);
// simulate incoming dmx data with this buffer

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

SPaG DMX

@peternewman

peternewman May 10, 2015

Member

SPaG DMX

DmxBuffer buffer;
@@ -508,12 +510,12 @@ void SPIOutputTest::testIndividualAPA102Control() {
buffer.SetFromString("7, 9");
output.WriteDMX(buffer);
data = backend.GetData(0, &length);
// check that the returns are the same as test2 (nothing changed)
// check that the returns are the same as test3 (nothing changed)
OLA_ASSERT_DATA_EQUALS(EXPECTED2, arraysize(EXPECTED2), data, length);
OLA_ASSERT_EQ(3u, backend.Writes(0));
// test5
// test with other StartAddress
// test with changed StartAddress
// set StartAddress
output.SetStartAddress(3);
// values 1 & 2 should not be visible in SPI data stream
@@ -526,12 +528,85 @@ void SPIOutputTest::testIndividualAPA102Control() {
0};
OLA_ASSERT_DATA_EQUALS(EXPECTED4, arraysize(EXPECTED4), data, length);
OLA_ASSERT_EQ(4u, backend.Writes(0));
// change StartAddress back to default
output.SetStartAddress(1);
// test6
// Check nothing changed on the other output.
OLA_ASSERT_EQ(reinterpret_cast<const uint8_t*>(NULL),
backend.GetData(1, &length));
OLA_ASSERT_EQ(0u, backend.Writes(1));
// test7
// create new output with pixel_count=16 and check data length
// setup pixel_count to 16
options.pixel_count = 16;
// 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.

buffer.SetFromString(
"0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0," +
"0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0,");
output.WriteDMX(buffer);
data = backend.GetData(0, &length);
const uint8_t EXPECTED4[] = { 0, 0, 0, 0,
0xff, 0, 0, 0, // Pixel 1
0xff, 0, 0, 0, // Pixel 2
0xff, 0, 0, 0, // Pixel 3
0xff, 0, 0, 0, // Pixel 4
0xff, 0, 0, 0, // Pixel 5
0xff, 0, 0, 0, // Pixel 6
0xff, 0, 0, 0, // Pixel 7
0xff, 0, 0, 0, // Pixel 8
0xff, 0, 0, 0, // Pixel 9
0xff, 0, 0, 0, // Pixel 10
0xff, 0, 0, 0, // Pixel 12
0xff, 0, 0, 0, // Pixel 13
0xff, 0, 0, 0, // Pixel 14
0xff, 0, 0, 0, // Pixel 15
0xff, 0, 0, 0, // Pixel 16
0};
OLA_ASSERT_DATA_EQUALS(EXPECTED4, arraysize(EXPECTED4), data, length);
OLA_ASSERT_EQ(5u, backend.Writes(0));
// test8
// create new output with pixel_count=17 and check data length
// setup pixel_count to 17
options.pixel_count = 17;
// setup SPIOutput
SPIOutput output(m_uid, &backend, options);
// set personality
output.SetPersonality(thisTestPersonality);
// generate dmx data
buffer.SetFromString(
"0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0," +
"0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0, 0,0,0," +
"0,0,0");
output.WriteDMX(buffer);
data = backend.GetData(0, &length);
const uint8_t EXPECTED4[] = { 0, 0, 0, 0,
0xff, 0, 0, 0, // Pixel 1
0xff, 0, 0, 0, // Pixel 2
0xff, 0, 0, 0, // Pixel 3
0xff, 0, 0, 0, // Pixel 4
0xff, 0, 0, 0, // Pixel 5
0xff, 0, 0, 0, // Pixel 6
0xff, 0, 0, 0, // Pixel 7
0xff, 0, 0, 0, // Pixel 8
0xff, 0, 0, 0, // Pixel 9
0xff, 0, 0, 0, // Pixel 10
0xff, 0, 0, 0, // Pixel 12
0xff, 0, 0, 0, // Pixel 13
0xff, 0, 0, 0, // Pixel 14
0xff, 0, 0, 0, // Pixel 15
0xff, 0, 0, 0, // Pixel 16
0xff, 0, 0, 0, // Pixel 17
0, 0}; // now we have two latch bytes...
OLA_ASSERT_DATA_EQUALS(EXPECTED4, arraysize(EXPECTED4), data, length);
OLA_ASSERT_EQ(6u, backend.Writes(0));
}
/**
ProTip! Use n and p to navigate between commits in a pull request.