Skip to content

Spi plugin port count - #1380

Merged
peternewman merged 8 commits into
OpenLightingProject:masterfrom
s-light:SPIPlugin_PortCount
Feb 25, 2018
Merged

Spi plugin port count#1380
peternewman merged 8 commits into
OpenLightingProject:masterfrom
s-light:SPIPlugin_PortCount

Conversation

@s-light

@s-light s-light commented Feb 20, 2018

Copy link
Copy Markdown
Contributor

fixes #1379
(based this on the APA102 Pixel Brightness Branch... hope that is ok)

@peternewman

Copy link
Copy Markdown
Member

Blocked behind #1358 .

Reviewed as s-light/ola@APA102PixelBrightness...s-light:SPIPlugin_PortCount .

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, other than that, LGTM.

Comment thread plugins/spi/SPIDevice.h
static const char SOFTWARE_BACKEND[];
static const uint16_t MAX_GPIO_PIN = 1023;
static const uint16_t MAX_PORT_COUNT = 32;
static const uint32_t MAX_SPI_SPEED = 32000000;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, can we order these to match the main file, i.e. speed first?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@peternewman

Copy link
Copy Markdown
Member

Would you mind addressing #1358 (comment) in here @s-light .

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as mentioned it would be ace to get #1358 (comment) sorted.

@s-light

s-light commented Feb 24, 2018

Copy link
Copy Markdown
Contributor Author

#1358 (comment)
done

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks.

@peternewman peternewman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One bug, as some of the tests are failing

Comment thread plugins/spi/SPIOutput.cpp Outdated
sd_irgb_combined.push_back(
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_INTENSITY, 0));
ola::rdm::SlotData::PrimarySlot(ola::rdm::SD_INTENSITY,
ola::rdm::DMX_MAX_SLOT_VALUE));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests, but curiously only the Linux ones, not OS X, are failing on this line, see https://travis-ci.org/OpenLightingProject/ola/jobs/345754263#L3298

You want ola::DMX_MAX_SLOT_VALUE, or probably just DMX_MAX_SLOT_VALUE with no namespace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry - did not run the compile&test (was late and iam tired and thought - hey just do it quick ;-) )
i fix it and this time first do the compiles myself before i commit ;-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, that's exactly what the Continuous Integration is there for, we all make mistakes; I certainly do it often enough!

@s-light

s-light commented Feb 25, 2018

Copy link
Copy Markdown
Contributor Author

done - for me it compiled and testsuite was successfully

@peternewman
peternewman merged commit 79c4fd7 into OpenLightingProject:master Feb 25, 2018
@s-light
s-light deleted the SPIPlugin_PortCount branch February 26, 2018 15:20
@peternewman peternewman added this to the 0.11.0 milestone Jun 12, 2018
@peternewman peternewman mentioned this pull request Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SPI Plugin: Port count maximum is 8

3 participants