Skip to content
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

Add default default octave depending on KK model #12

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nrb0
Copy link
Collaborator

@nrb0 nrb0 commented Feb 10, 2018

Removes the concept of templated KKS1 child class. The number of keys is an argument of the base class
This is done in order to initialize the m_firstOctave to the default keyboard value depending (which depends on the size)


This change is Reviewable

@nrb0 nrb0 requested a review from shaduzlabs February 10, 2018 11:51
KompleteKontrolBase::KompleteKontrolBase(const NUM_KEYS numKeys)
: m_numKeys(static_cast<unsigned>(numKeys))
, m_ledsKeysSize(m_numKeys * 3U)
, m_ledsKeys(new uint8_t[m_ledsKeysSize])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

c style array creation, since you're using pointers for data writing I didn't change this to a vector

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe a shared_ptr with the array is an option here

@@ -311,6 +332,7 @@ KompleteKontrolBase::KompleteKontrolBase()

KompleteKontrolBase::~KompleteKontrolBase()
{
delete[] m_ledsKeys;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

delete based destruction. A bit ugly, maybe there's a better way to do this

@@ -22,6 +22,23 @@ const uint8_t kKK_ledsDataSize = 25;

const uint8_t kKK_epOut = 0x02;
const uint8_t kKK_epInput = 0x84;

size_t getInitialOctave(const sl::cabl::KompleteKontrolBase::NUM_KEYS numKeys)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe there's a better name for this

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.176% when pulling 42ec2a9 on feature/Small_key_refactor into 7a86f43 on develop.

@nrb0 nrb0 changed the title Refactor KKS base class, remove templated numKeys Add default default octave depending on KK model Jun 19, 2018
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.

None yet

3 participants