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.
+423 −20
Diff settings

Always

Just for now

View
@@ -23,6 +23,10 @@ Thumbs.db
*/*/Thumbs.db
*/*/*/.dirstamp
*/*/*/Thumbs.db
.DS_Store
*.bak
**/nppBackup
.~lock.*
INSTALL
Makefile
Makefile.in
View
@@ -86,9 +86,14 @@ const uint8_t SPIOutput::SPI_MODE = 0;
const uint16_t SPIOutput::WS2801_SLOTS_PER_PIXEL = 3;
const uint16_t SPIOutput::LPD8806_SLOTS_PER_PIXEL = 3;
const uint16_t SPIOutput::P9813_SLOTS_PER_PIXEL = 3;
const uint16_t SPIOutput::APA102_SLOTS_PER_PIXEL = 3;
// Number of bytes that each P9813 pixel uses on the spi wires
// Number of bytes that each pixel uses on the SPI wires
// (if it differs from 1:1 with colors)
const uint16_t SPIOutput::P9813_SPI_BYTES_PER_PIXEL = 4;
const uint16_t SPIOutput::APA102_SPI_BYTES_PER_PIXEL = 4;
const uint16_t SPIOutput::APA102_START_FRAME_BYTES = 4;
SPIOutput::RDMOps *SPIOutput::RDMOps::instance = NULL;
@@ -187,6 +192,10 @@ SPIOutput::SPIOutput(const UID &uid, SPIBackendInterface *backend,
"P9813 Individual Control"));
personalities.push_back(Personality(P9813_SLOTS_PER_PIXEL,
"P9813 Combined Control"));
personalities.push_back(Personality(m_pixel_count * APA102_SLOTS_PER_PIXEL,
"APA102 Individual Control"));
personalities.push_back(Personality(APA102_SLOTS_PER_PIXEL,
"APA102 Combined Control"));
m_personality_collection.reset(new PersonalityCollection(personalities));
m_personality_manager.reset(new PersonalityManager(
m_personality_collection.get()));
@@ -301,6 +310,12 @@ bool SPIOutput::InternalWriteDMX(const DmxBuffer &buffer) {
case 6:
CombinedP9813Control(buffer);
break;
case 7:
IndividualAPA102Control(buffer);
break;
case 8:
CombinedAPA102Control(buffer);
break;
default:
break;
}
@@ -486,6 +501,121 @@ uint8_t SPIOutput::P9813CreateFlag(uint8_t red, uint8_t green, uint8_t blue) {
return ~flag;
}
void SPIOutput::IndividualAPA102Control(const DmxBuffer &buffer) {
// 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

// LEDFrame: 1Byte FF ; 3Byte color info (Blue, Green, Red)
// EndFrame: (n/2)bits; n = pixel_count
// 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
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
OLA_INFO << "Insufficient DMX data, required " << APA102_SLOTS_PER_PIXEL
<< ", got " << buffer.Size() - first_slot;
return;

This comment has been minimized.

@peternewman

peternewman May 10, 2015

Member

Can you add the OLA_INFO statement you've got in the same check within Combined please.

@peternewman

peternewman May 10, 2015

Member

Can you add the OLA_INFO statement you've got in the same check within Combined please.

}

This comment has been minimized.

@s-light

s-light May 13, 2015

Contributor

just a style question:
from my feeling i would write this in 'positiv-logic' so i do not have multiple breakout/return points in the function -

// only do something if minimum 1pixel can be updated..
if ((buffer.Size() - first_slot) >= APA102_SLOTS_PER_PIXEL) {
  // the ongoing function
  .....
} else {
  OLA_INFO << "Insufficient DMX data, required " << APA102_SLOTS_PER_PIXEL
           << ", got " << buffer.Size() - first_slot;
}

how do you prefer it?

@s-light

s-light May 13, 2015

Contributor

just a style question:
from my feeling i would write this in 'positiv-logic' so i do not have multiple breakout/return points in the function -

// only do something if minimum 1pixel can be updated..
if ((buffer.Size() - first_slot) >= APA102_SLOTS_PER_PIXEL) {
  // the ongoing function
  .....
} else {
  OLA_INFO << "Insufficient DMX data, required " << APA102_SLOTS_PER_PIXEL
           << ", got " << buffer.Size() - first_slot;
}

how do you prefer it?

This comment has been minimized.

@peternewman

peternewman May 13, 2015

Member

Very much a @nomis52 one. I'd generally prefer what you're suggesting, but most/all of the C++ code uses the early returns. I guess the main upside is its easier to read than a few levels of nested ifs.

@peternewman

peternewman May 13, 2015

Member

Very much a @nomis52 one. I'd generally prefer what you're suggesting, but most/all of the C++ code uses the early returns. I guess the main upside is its easier to read than a few levels of nested ifs.

This comment has been minimized.

@nomis52

nomis52 May 14, 2015

Member

We prefer early returns for the reasons Peter mentioned.

@nomis52

nomis52 May 14, 2015

Member

We prefer early returns for the reasons Peter mentioned.

This comment has been minimized.

@peternewman

peternewman May 28, 2015

Member

@s-light has sorted this now.

@peternewman

peternewman May 28, 2015

Member

@s-light has sorted this now.

// We always check out the entire string length, even if we only have data
// for part of it
const unsigned int output_length = APA102_START_FRAME_BYTES +
(m_pixel_count * APA102_SPI_BYTES_PER_PIXEL);
uint8_t *output = m_backend->Checkout(m_output_number, output_length,
CalculateAPA102LatchBytes(m_pixel_count));
// only update SPI data if possible
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

for (unsigned int i = 0; i < m_pixel_count; i++) {
// Convert RGB to APA102 Pixel
unsigned int offset = first_slot + (i * APA102_SLOTS_PER_PIXEL);
// We need to avoid the first 4 bytes of the buffer since that acts as a
// start of frame delimiter
unsigned int spi_offset = APA102_START_FRAME_BYTES +
(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

// 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

// 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

// only write pixel data if buffer has complete data for this pixel:
if ((buffer.Size() - offset) >= APA102_SLOTS_PER_PIXEL) {
// Convert RGB to APA102 Pixel
// 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.

}
}
m_backend->Commit(m_output_number);
}
void SPIOutput::CombinedAPA102Control(const DmxBuffer &buffer) {
// for Protocol details see IndividualAPA102Control
const uint16_t first_slot = m_start_address - 1; // 0 offset
// check if enough data is there.
if (buffer.Size() - first_slot < APA102_SLOTS_PER_PIXEL) {
OLA_INFO << "Insufficient DMX data, required " << APA102_SLOTS_PER_PIXEL
<< ", got " << buffer.Size() - first_slot;
return;
}
// 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

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

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

// only update SPI data if possible
if (!output) {
return;
}
// create Pixel Data
uint8_t pixel_data[APA102_SPI_BYTES_PER_PIXEL];
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); // 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 :-)

// 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);
memcpy(&output[spi_offset], pixel_data,
APA102_SPI_BYTES_PER_PIXEL);
}
// write output back...
m_backend->Commit(m_output_number);
}
/**
* Calculate Latch Bytes for APA102:
* 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. (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(uint16_t pixel_count) {
// round up so that we get definitely enough bits
const uint8_t latch_bits = (pixel_count + 1) / 2;
const uint8_t latch_bytes = (latch_bits + 7) / 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.

RDMResponse *SPIOutput::GetDeviceInfo(const RDMRequest *request) {
return ResponderHelper::GetDeviceInfo(
request, ola::rdm::OLA_SPI_DEVICE_MODEL,
View
@@ -104,12 +104,16 @@ class SPIOutput: public ola::rdm::DiscoverableRDMControllerInterface {
// DMX methods
bool InternalWriteDMX(const DmxBuffer &buffer);
void IndividualWS2801Control(const DmxBuffer &buffer);
void CombinedWS2801Control(const DmxBuffer &buffer);
void IndividualLPD8806Control(const DmxBuffer &buffer);
void CombinedLPD8806Control(const DmxBuffer &buffer);
void IndividualP9813Control(const DmxBuffer &buffer);
void CombinedP9813Control(const DmxBuffer &buffer);
void IndividualAPA102Control(const DmxBuffer &buffer);
void CombinedAPA102Control(const DmxBuffer &buffer);
unsigned int LPD8806BufferSize() const;
void WriteSPIData(const uint8_t *data, unsigned int length);
@@ -169,6 +173,7 @@ class SPIOutput: public ola::rdm::DiscoverableRDMControllerInterface {
// Helpers
uint8_t P9813CreateFlag(uint8_t red, uint8_t green, uint8_t blue);
static uint8_t CalculateAPA102LatchBytes(uint16_t pixel_count);
static const uint8_t SPI_MODE;
static const uint8_t SPI_BITS_PER_WORD;
@@ -178,6 +183,9 @@ class SPIOutput: public ola::rdm::DiscoverableRDMControllerInterface {
static const uint16_t LPD8806_SLOTS_PER_PIXEL;
static const uint16_t P9813_SLOTS_PER_PIXEL;
static const uint16_t P9813_SPI_BYTES_PER_PIXEL;
static const uint16_t APA102_SLOTS_PER_PIXEL;
static const uint16_t APA102_SPI_BYTES_PER_PIXEL;
static const uint16_t APA102_START_FRAME_BYTES;
static const ola::rdm::ResponderOps<SPIOutput>::ParamHandler
PARAM_HANDLERS[];
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.