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

added APA102 with Pixel Brightness mode #1358

Merged
merged 53 commits into from Feb 24, 2018

Conversation

Projects
None yet
3 participants
@s-light
Contributor

s-light commented Jan 5, 2018

this fixes #1356
make check passed all tests.
hardware test i to be done. i will report back if i have it tested in real live :-)

@peternewman peternewman self-assigned this Jan 5, 2018

@peternewman peternewman added this to the 0.11.0 milestone Jan 5, 2018

@peternewman

A few comments, mostly fairly minor.

It would be nice to have a CombinedAPA102ControlPixelBrightness personality for completeness, i.e. 4 channels controlling a whole strip.

If possible in your hardware tests, if possible it would be good to check two blocks, one in PB mode, one not, work when driven from the same plugin.

// https://cpldcpu.wordpress.com/2014/11/30/understanding-the-apa102-superled/
// Data-Struct
// StartFrame: 4 bytes = 32 bits zeros (APA102_START_FRAME_BYTES)
// LEDFrame: 1 byte FF ; 3 bytes color info (Blue, Green, Red)

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

This comment ought to have details on the brightness, rather than FF

uint8_t pixel_brightness = CalculateAPA102PixelBrightness(
buffer.Get(offset + 0));
// merge 3 bits start mark (111): can be written as 0xE0
uint8_t first_byte = 0xE0 && pixel_brightness;

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

Can we make 0xE0 a constant please

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

You could avoid storing pixel_brightness and just && it straight in.

This comment has been minimized.

@peternewman

peternewman Jan 6, 2018

Member

Also as Codacy put it "Condition '224' is always true", it should be a single ampersand for bitwise AND. I'm kind of surprised nothing else picked this up!

// merge 3 bits start mark (111): can be written as 0xE0
uint8_t first_byte = 0xE0 && pixel_brightness;
// set first_byte to output buffer
output[spi_offset + 0] = first_byte;

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

Likewise for first_byte.

// brightness == 255
// x == 31
// x = brightness * 31 / 255
return brightness * 31 / 255;

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

This can just be done as (brightness >> 3) and discard the lower order bits.

* Test DMX writes in the individual APA102 Pixel Brightness mode.
*/
void SPIOutputTest::testIndividualAPA102ControlPixelBrightness() {
// personality 7= Individual APA102

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

This should be personality 9, 7 is individual no PB.

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

Thinking about trying to fix this, I don't know what @nomis52 thinks, but to me it would be good if we had enums for the personalties, as then this problem, and the switch case to decide what mode we're in can be neater.

If we list the enums, then we can do
personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL, Personality("WS2801 Individual"));

Then switch/run tests based on PERS_WS2801_INDIVIDUAL.

Or we could build an array and convert it to a vector.

*/
void SPIOutputTest::testIndividualAPA102ControlPixelBrightness() {
// personality 7= Individual APA102
const uint16_t this_test_personality = 7;

This comment has been minimized.

@peternewman
output3.WriteDMX(buffer);
data = backend.GetData(0, &length);
const uint8_t EXPECTED10[] = { 0, 0, 0, 0,
0xE0 && 0, 0, 0, 0, // Pixel 1

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

This test can't be working with the selected personality, or there's a bigger issue.

This comment has been minimized.

@peternewman

peternewman Jan 5, 2018

Member

Add that issue is you've not added it via the CPPUNIT_TEST macro at the top of the file, so it's not actually being run, hence the tests are still passing.

This comment has been minimized.

@peternewman

peternewman Jan 6, 2018

Member

Also && here.

@peternewman

This comment has been minimized.

Member

peternewman commented Jan 5, 2018

You should also increment the RDM software version @s-light when you add a personality (the 4 in GetDeviceInfo) in SPIOutput.cpp.

peternewman and others added some commits Jan 6, 2018

@s-light

This comment has been minimized.

Contributor

s-light commented Jan 6, 2018

thanks @peternewman for your comments - i found today that i did forget to include the test.... ;-)
the && is i think coming from a mix of languages..
and you are right the test is not passing at all.....
i will work this over - but don't know if i get to do it the next days - iam now on vacation for a week - have to see..

@peternewman

This comment has been minimized.

Member

peternewman commented Jan 6, 2018

Which one uses && for bitwise AND then, as opposed to logical, or does it use the same for both?

The test isn't passing, because as per one of my other comments, you're using the wrong personality, so you're not generating data via your code, but trying to test to match it.

s-light and others added some commits Jan 6, 2018

fixed first test
TODO: fix other tests
@s-light

This comment has been minimized.

Contributor

s-light commented Jan 8, 2018

now make check is successful :-)
tomorrow i will have a look at the APA102_LEDFRAME_START_MARKdeclaration.
and if this is fixed i will have a try at your recommendation regarding the enum for the personalities

@peternewman

Mostly just style issues now thanks s-light.

personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,
"WS2801 Individual Control"));

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Can we align the descriptions with the ( on the line above please.

This comment has been minimized.

@peternewman

peternewman Jan 27, 2018

Member

Yeah so both lines still need to go in by one please.

// personality description is max 32 characters
personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Can we indent this to align with the ( above ideally, or otherwise four in from personalities depending on line length.

This comment has been minimized.

@s-light

s-light Jan 27, 2018

Contributor

do you mean like this:

  personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
                       Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,
                                   "WS2801 Individual Control"));

or like this:
(here all indentations are multiples of 2 from the line-start on)

  personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
                      Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,
                                  "WS2801 Individual Control"));

both would fit the max line length.

This comment has been minimized.

@peternewman

peternewman Jan 27, 2018

Member

The top one please, it's the two arguments to insert, then the two to Personality.

ola::rdm::SlotDataCollection::SlotDataList sd_APA102_COMBINED;
sd_APA102_COMBINED.push_back(
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_RED, 0));

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Indent to four please.

This comment has been minimized.

@s-light

s-light Jan 27, 2018

Contributor

you mean so:

  sd_APA102_COMBINED.push_back(
      ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_RED, 0));

i thought that the indentation rule is 2 spaces.
or is it 2spaces - and on continuing lines 4spaces?
sorry to mess up this things!

This comment has been minimized.

@peternewman

peternewman Jan 27, 2018

Member

Yeah, we indent by two (apart from public:/private:, which are one where we've bothered/in some cases), but four for a continuing line.

This comment has been minimized.

@peternewman
@@ -375,7 +428,7 @@ void SPIOutput::IndividualLPD8806Control(const DmxBuffer &buffer) {
if (!output)
return;
const unsigned int length = std::min(m_pixel_count * LPD8806_SLOTS_PER_PIXEL,
const unsigned int length = min(m_pixel_count * LPD8806_SLOTS_PER_PIXEL,

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

I think this wants to be back to std:: unless you've done a using.

This comment has been minimized.

@s-light

s-light Jan 27, 2018

Contributor

i deleted the using and added the namespace things like std:: first because i misinterpreted your comment... 26c5181
then re-add it. ceea93d
i don't know what you are preferring (and i don't know what is 'good style' here..)

This comment has been minimized.

@peternewman

peternewman Jan 27, 2018

Member

I think the vague theory is use using if we've got a few hits, but probably not if we've only got one, as it pollutes the namespace a little, as well as making things marginally less clear. i.e. the eternal balance of being concise versus clear.

This file seems to have a "using std::auto_ptr;" and no other mention of auto_ptr, so that's a good start. But then we have vector once, but using. So we have no hard and fast rules, on the .cpp files anyway; it's more critical in the header files, although I can't remember exactly why off the top of my head.

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

Sorry @s-light bump, can we remove the unused "using std::auto_ptr;" please (it's too far up the file for me to comment). I realise it's not your problem either!

@@ -579,6 +632,82 @@ void SPIOutput::IndividualAPA102Control(const DmxBuffer &buffer) {
m_backend->Commit(m_output_number);
}
void SPIOutput::IndividualAPA102ControlPixelBrightness(
const DmxBuffer &buffer

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Indent by four please.

This comment has been minimized.

@peternewman
// 3 bits start mark (111) (APA102_LEDFRAME_START_MARK) +
// 5 bits pixel brightness (datasheet name: global brightness)
output[spi_offset + 0] = SPIOutput::APA102_LEDFRAME_START_MARK |
CalculateAPA102PixelBrightness(buffer.Get(offset + 0));

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Indent to four. Can we have brackets round the bitwise or operation please.

This comment has been minimized.

@peternewman
const uint16_t first_slot = m_start_address - 1; // 0 offset
// check if enough data is there.
if (buffer.Size() - first_slot < APA102_PB_SLOTS_PER_PIXEL) {

This comment has been minimized.

@peternewman

This comment has been minimized.

@peternewman
// 3 bits start mark (111) (APA102_LEDFRAME_START_MARK) +
// 5 bits pixel brightness (datasheet name: global brightness)
pixel_data[0] = SPIOutput::APA102_LEDFRAME_START_MARK |
CalculateAPA102PixelBrightness(buffer.Get(first_slot + 0));

This comment has been minimized.

@peternewman

This comment has been minimized.

@peternewman
@@ -689,7 +892,9 @@ RDMResponse *SPIOutput::SetDeviceLabel(const RDMRequest *request) {
}
RDMResponse *SPIOutput::GetSoftwareVersionLabel(const RDMRequest *request) {
return ResponderHelper::GetString(request, string("OLA Version ") + VERSION);

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Did this need changing? Or is this an editor change, which might explain a lot of my other comments.

This comment has been minimized.

@s-light

s-light Jan 27, 2018

Contributor

both no - i did it (as i thought its easier to read this way - i will revoke it..)

This comment has been minimized.

@peternewman

peternewman Jan 27, 2018

Member

I don't really mind, although compactness often helps given we don't actually care about that line. If it's going to be on multiple lines, we generally live the first argument with the function/method, then indent the rest to match, unless one of the lines is too long. But we normally squeeze a lot onto the line until we hit the line length.

// new ones can be added at end.
// remember to increment RDM-Version in
// SPIOutput.cpp SPIOutput::GetDeviceInfo()
// enum class SPI_PERSONALITY : unsigned int {

This comment has been minimized.

@peternewman

peternewman Jan 26, 2018

Member

Can we drop this comment line please.

This comment has been minimized.

@peternewman
@peternewman

Just a couple more minor style changes please.

// personality description is max 32 characters
personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,

This comment has been minimized.

@peternewman

peternewman Feb 6, 2018

Member

These still want indenting please, like so (i.e. add one space to the second and third lines):

personalities.insert(personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,
                     Personality(m_pixel_count * WS2801_SLOTS_PER_PIXEL,
                                 "WS2801 Individual Control"));

This comment has been minimized.

@peternewman
personalities.insert(personalities.begin() + PERS_APA102_COMBINED - 1,
Personality(APA102_SLOTS_PER_PIXEL,
"APA102 Combined Control",
ola::rdm::SlotDataCollection(sd_APA102_COMBINED)));

This comment has been minimized.

@peternewman

peternewman Feb 6, 2018

Member

The line ola::rdm::SlotDataCollection(sd_APA102_COMBINED) can become sd_APA102_COMBINED.

This comment has been minimized.

@peternewman

This comment has been minimized.

@s-light

s-light Feb 19, 2018

Contributor

hm - i was to fast with the commit -
for me this results in an error:

no known conversion for argument 3 from ‘ola::rdm::SlotDataCollection::SlotDataList {aka std::vector<ola::rdm::SlotData>}’ to ‘const ola::rdm::SlotDataCollection&’

plugins/spi/SPIOutput.cpp: In constructor ‘ola::plugin::spi::SPIOutput::SPIOutput(const ola::rdm::UID&, ola::plugin::spi::SPIBackendInterface*, const ola::plugin::spi::SPIOutput::Options&)’:
plugins/spi/SPIOutput.cpp:225:53: error: no matching function for call to ‘ola::rdm::Personality::Personality(const uint16_t&, const char [24], ola::rdm::SlotDataCollection::SlotDataList&)’
                                   sd_APA102_COMBINED));
                                                     ^
In file included from ./include/ola/rdm/ResponderHelper.h:35:0,
                 from plugins/spi/SPIOutput.cpp:42:
./include/ola/rdm/ResponderPersonality.h:41:5: note: candidate: ola::rdm::Personality::Personality(uint16_t, const string&, const ola::rdm::SlotDataCollection&)
     Personality(uint16_t footprint, const std::string &description,
     ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:41:5: note:   no known conversion for argument 3 from ‘ola::rdm::SlotDataCollection::SlotDataList {aka std::vector<ola::rdm::SlotData>}’ to ‘const ola::rdm::SlotDataCollection&’
./include/ola/rdm/ResponderPersonality.h:40:5: note: candidate: ola::rdm::Personality::Personality(uint16_t, const string&)
     Personality(uint16_t footprint, const std::string &description);
     ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:40:5: note:   candidate expects 2 arguments, 3 provided
./include/ola/rdm/ResponderPersonality.h:38:7: note: candidate: ola::rdm::Personality::Personality(const ola::rdm::Personality&)
 class Personality {
       ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:38:7: note:   candidate expects 1 argument, 3 provided
./include/ola/rdm/ResponderPersonality.h:38:7: note: candidate: ola::rdm::Personality::Personality(ola::rdm::Personality&&)
./include/ola/rdm/ResponderPersonality.h:38:7: note:   candidate expects 1 argument, 3 provided
plugins/spi/SPIOutput.cpp:243:57: error: no matching function for call to ‘ola::rdm::Personality::Personality(const uint16_t&, const char [33], ola::rdm::SlotDataCollection::SlotDataList&)’
                                    sd_APA102_PB_COMBINED));
                                                         ^
In file included from ./include/ola/rdm/ResponderHelper.h:35:0,
                 from plugins/spi/SPIOutput.cpp:42:
./include/ola/rdm/ResponderPersonality.h:41:5: note: candidate: ola::rdm::Personality::Personality(uint16_t, const string&, const ola::rdm::SlotDataCollection&)
     Personality(uint16_t footprint, const std::string &description,
     ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:41:5: note:   no known conversion for argument 3 from ‘ola::rdm::SlotDataCollection::SlotDataList {aka std::vector<ola::rdm::SlotData>}’ to ‘const ola::rdm::SlotDataCollection&’
./include/ola/rdm/ResponderPersonality.h:40:5: note: candidate: ola::rdm::Personality::Personality(uint16_t, const string&)
     Personality(uint16_t footprint, const std::string &description);
     ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:40:5: note:   candidate expects 2 arguments, 3 provided
./include/ola/rdm/ResponderPersonality.h:38:7: note: candidate: ola::rdm::Personality::Personality(const ola::rdm::Personality&)
 class Personality {
       ^~~~~~~~~~~
./include/ola/rdm/ResponderPersonality.h:38:7: note:   candidate expects 1 argument, 3 provided
./include/ola/rdm/ResponderPersonality.h:38:7: note: candidate: ola::rdm::Personality::Personality(ola::rdm::Personality&&)
./include/ola/rdm/ResponderPersonality.h:38:7: note:   candidate expects 1 argument, 3 provided

This comment has been minimized.

@peternewman

peternewman Feb 19, 2018

Member

Oops, my apologies, sorry about that. Perhaps we should have a Personality constructor that takes a vector directly?

This comment has been minimized.

@s-light

s-light Feb 19, 2018

Contributor

seems like a good idea ;-)
i can try to look at this next week.
would it be enough to add a additional overloaded constructor and from this one use the cast and call the current one? (i have not looked at the code yet..)

This comment has been minimized.

@peternewman

peternewman Feb 19, 2018

Member

Yes I think so, but don't trust what I said given the earlier comment! :)

personalities.insert(personalities.begin() + PERS_APA102_PB_COMBINED - 1,
Personality(APA102_PB_SLOTS_PER_PIXEL,
"APA102 Pixel Brightness Combined",
ola::rdm::SlotDataCollection(sd_APA102_PB_COMBINED)));

This comment has been minimized.

@peternewman

This comment has been minimized.

@peternewman
@@ -642,6 +658,13 @@ void SPIOutputTest::testCombinedAPA102Control() {
unsigned int length = 0;
const uint8_t *data = NULL;
// test0

This comment has been minimized.

@peternewman

peternewman Feb 6, 2018

Member

Can we remove this please.

This comment has been minimized.

@peternewman
@peternewman

This comment has been minimized.

Member

peternewman commented Feb 19, 2018

Do you think you've got time to do these last few minor style changes soon @s-light so we can get this merged in, as I suspect 0.11.0 may finally appear soon...

@s-light

This comment has been minimized.

Contributor

s-light commented Feb 19, 2018

i try to do it as soon as possible. (hopefully i get to it until Thursday - iam a bit distracted by a project that i want to show at Saturday at an MiniMakerFaire..)

@s-light

This comment has been minimized.

Contributor

s-light commented Feb 19, 2018

i try to do a real-live hardware test in the next days.
(i have done some minimal testing a few commits ago on real hardware - back than it worked..)

s-light added some commits Feb 19, 2018

@s-light

This comment has been minimized.

Contributor

s-light commented Feb 20, 2018

real-world-test done :-)
http://blog.s-light.eu/ola-apa102-pixelbrightness-support/
(made a short video of a 144pixel strip controlled by two universes -
input both E1.31 (sACN) output first 100 leds with APA102 PB individ. rest 44 leds with APA102 combined. so i also tested the old and the new in combination.. (tested other combinations without video...)
for me it seems that it is working fine.

@peternewman

Just a few more minor comments.

Can we get the style and comment removal stuff done. The slot data changes could wait until a future PR or someone else if you'd rather.

@@ -40,13 +40,21 @@ using std::string;
Personality::Personality(uint16_t footprint, const string &description)
: m_footprint(footprint),
m_description(description) {
// STATIC_ASSERT(description.length() <= 32);

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

I'm assuming this line doesn't work at all, in which case can we remove it please?

This comment has been minimized.

@peternewman
@@ -40,13 +40,21 @@ using std::string;
Personality::Personality(uint16_t footprint, const string &description)
: m_footprint(footprint),
m_description(description) {
// STATIC_ASSERT(description.length() <= 32);
// c++11:
// static_assert(

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

Does this work in C++11? If so I guess we can leave it as a comment, but perhaps make the C++11 line a TODO(C++11) or something.

This comment has been minimized.

@peternewman
}
Personality::Personality(uint16_t footprint, const string &description,
const SlotDataCollection &slot_data)
: m_footprint(footprint),
m_description(description),
m_slot_data(slot_data) {
// constexpr int descriptionlen = description.length();

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

Can we remove this too if it doesn't work.

This comment has been minimized.

@peternewman
Personality(m_pixel_count * APA102_SLOTS_PER_PIXEL,
"APA102 Individual Control"));
ola::rdm::SlotDataCollection::SlotDataList sd_APA102_COMBINED;

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

I was going to say lower case some/all of this variable name, but given we've made it, why don't we stick it at the top and call it sd_rgb_combined, then we can apply it to all the RGB combined SPI pixels.

Which also means my suggestion about a Personality constructor taking a SlotDataList becomes redundant, as we want to convert it to a SlotDataCollection once, then use that throughout.

This comment has been minimized.

@peternewman
Personality(m_pixel_count * APA102_PB_SLOTS_PER_PIXEL,
"APA102 Pixel Brightness Individ."));
ola::rdm::SlotDataCollection::SlotDataList sd_APA102_PB_COMBINED;

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

Likewise for the irgb one.

This comment has been minimized.

@peternewman
personalities.insert(personalities.begin() + PERS_APA102_COMBINED - 1,
Personality(APA102_SLOTS_PER_PIXEL,
"APA102 Combined Control",
ola::rdm::SlotDataCollection(sd_APA102_COMBINED)));

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

This wants to align with the Personality( bracket, or be offset, so you can do:

  personalities.insert(personalities.begin() + PERS_APA102_COMBINED - 1,
                       Personality(
                           APA102_SLOTS_PER_PIXEL,
                           "APA102 Combined Control",
                           ola::rdm::SlotDataCollection(sd_APA102_COMBINED)));

Or:

  personalities.insert(
      personalities.begin() + PERS_APA102_COMBINED - 1,
      Personality(APA102_SLOTS_PER_PIXEL,
                  "APA102 Combined Control",
                  ola::rdm::SlotDataCollection(sd_APA102_COMBINED)));

Whichever you fancy, although the second looks nicer to me I think.

This comment has been minimized.

@peternewman
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_GREEN, 0));
sd_APA102_COMBINED.push_back(
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_BLUE, 0));
personalities.insert(personalities.begin() + PERS_APA102_COMBINED - 1,

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

As below, sorry I got the wrong order.

This comment has been minimized.

@peternewman
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_GREEN, 0));
sd_APA102_PB_COMBINED.push_back(
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_COLOR_ADD_BLUE, 0));
personalities.insert(personalities.begin() + PERS_APA102_PB_COMBINED - 1,

This comment has been minimized.

@peternewman

This comment has been minimized.

@peternewman
@@ -375,7 +428,7 @@ void SPIOutput::IndividualLPD8806Control(const DmxBuffer &buffer) {
if (!output)
return;
const unsigned int length = std::min(m_pixel_count * LPD8806_SLOTS_PER_PIXEL,
const unsigned int length = min(m_pixel_count * LPD8806_SLOTS_PER_PIXEL,

This comment has been minimized.

@peternewman

peternewman Feb 21, 2018

Member

Sorry @s-light bump, can we remove the unused "using std::auto_ptr;" please (it's too far up the file for me to comment). I realise it's not your problem either!

@peternewman peternewman referenced this pull request Feb 21, 2018

Merged

Spi plugin port count #1380

sdc_irgb_combined = ola::rdm::SlotDataCollection(sd_irgb_combined);
personalities.insert(
personalities.begin() + PERS_WS2801_INDIVIDUAL - 1,

This comment has been minimized.

@peternewman

peternewman Feb 22, 2018

Member

You don't need to move this one down, but I understand you may want to make them all look the same.

This comment has been minimized.

@s-light

s-light Feb 22, 2018

Contributor

you understand my intention ;-)
i think its much easier to read if in one code section its the same format/line-breaking...
i like the 'list-style' this way most - but i know you mentioned the rule is normally 'all that fits in the same line'...
if you want i change it back for the short ones...

This comment has been minimized.

@peternewman

peternewman Feb 24, 2018

Member

That's a good enough reason for me! Rule is probably too strong, general style, but with enough exceptions you can do what you want. 😄

@peternewman

Just one minor comment I think we ought to fix, but LGTM, so happy if it gets merged without that (or pushed into #1380 or something).

ola::rdm::SlotDataCollection::SlotDataList sd_irgb_combined;
sd_irgb_combined.push_back(
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_INTENSITY, 0));

This comment has been minimized.

@peternewman

peternewman Feb 24, 2018

Member

This probably wants to default to 100% aka 255 aka DMX_MAX_SLOT_VALUE, so by default it still just works like RGB mode, but with the option of using the overall intensity.

@peternewman peternewman merged commit bec8cf2 into OpenLightingProject:master Feb 24, 2018

2 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@s-light s-light deleted the s-light:APA102PixelBrightness branch Feb 24, 2018

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