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 support for input shift registers (e.g. 74HC165) #77

Merged
merged 22 commits into from
Jan 5, 2022

Conversation

neilenns
Copy link
Contributor

Description of changes

Fixes #73

Adds support for input shift registers like the 74HC165.

@@ -0,0 +1,56 @@
// MFInputShifter.h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The general shape of the MFInputShifter is modeled on the existing output shifter and button classes.

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip


// Multiple chained modules are handled one at a time. As shiftIn() keeps getting
// called it will pull in the data from each chained module.
for (int i = 0; i < _moduleCount; i++)
Copy link
Contributor Author

@neilenns neilenns Oct 29, 2021

Choose a reason for hiding this comment

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

This is a different implementation approach than the one used for output shifters that I think is easier to understand.


// Triggers the event handler for the associated input shift register pin,
// if a handler is registered.
void MFInputShifter::trigger(uint8_t pin, bool state)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is identical to how buttons work other than the addition of sending the pin that triggered along with the event.

@@ -439,6 +455,43 @@ void ClearEncoders()
#endif
}

#if MF_INPUT_SHIFTER_SUPPORT == 1
//// INPUT SHIFT REGISTER /////
void AddInputShifter(uint8_t latchPin, uint8_t clockPin, uint8_t dataPin, uint8_t modules, char const *name = "Shifter")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a mashup of how output shifters and buttons are configured.

Copy link
Contributor

Choose a reason for hiding this comment

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

Issue warning: The name argument is not used anywhere in the function, therefore every shifter retains the same, default, name. All shifter chains will therefore send the same message for a given bit.
Currently, the "name" field of the object is initialized implicitly (to "InputShifter") during instantiation, and then it can no longer be modified; a method should be added to class MFInputShifter (e.g. MFInputShifter::setName(const char *newName) ) and invoked here.
Buttons behave differently because they are instantiated one by one, and thus named, upon registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This is fixed now, it just required assigning the name in the attach method, as is done with the other modules.

Copy link
Contributor

@GioCC GioCC Nov 5, 2021

Choose a reason for hiding this comment

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

Of course, that makes more sense than a separate method!
(Sorry for adding my comment twice, I didn't notice I had already confirmed the first entry.)

@neilenns neilenns requested a review from elral October 29, 2021 04:30
@neilenns neilenns marked this pull request as draft October 29, 2021 04:30
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

src/mobiflight.cpp Outdated Show resolved Hide resolved
@elral
Copy link
Collaborator

elral commented Oct 31, 2021

No further coments to this pull request.

Just a general remark. Maybe we should think about a "real" debouncing routine and not to use just a waiting time. I am wondering if there were "problems" with bouncing buttons reported in the past .

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

Copy link
Collaborator

@elral elral left a comment

Choose a reason for hiding this comment

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

From my side everything OK

@@ -439,6 +455,43 @@ void ClearEncoders()
#endif
}

#if MF_INPUT_SHIFTER_SUPPORT == 1
//// INPUT SHIFT REGISTER /////
void AddInputShifter(uint8_t latchPin, uint8_t clockPin, uint8_t dataPin, uint8_t modules, char const *name = "Shifter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue warning: The name argument is not used anywhere in the function, therefore every shifter retains the same, default, name. All shifter chains will therefore send the same message for a given bit.
Currently, the "name" field of the object is initialized implicitly (to "InputShifter") during instantiation, and then it can no longer be modified; a method should be added to class MFInputShifter (e.g. MFInputShifter::setName(const char *newName) ) and invoked here.
Buttons behave differently because they are instantiated one by one, and thus named, upon registration.

@github-actions
Copy link

github-actions bot commented Nov 5, 2021

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns marked this pull request as ready for review November 21, 2021 02:04
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

Firmware for this pull request:
Firmware.zip

GioCC pushed a commit to GioCC/MobiFlight-FirmwareSource that referenced this pull request Dec 20, 2021
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

Copy link
Collaborator

@DocMoebiuz DocMoebiuz left a comment

Choose a reason for hiding this comment

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

Thanks for your effort! Looks good.

Let us please rename the original shifter to MFOutputShifter to have this analogy perfect.
That would be awesome!

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Firmware for this pull request:
Firmware.zip

@neilenns
Copy link
Contributor Author

neilenns commented Jan 5, 2022

Let us please rename the original shifter to MFOutputShifter to have this analogy perfect. That would be awesome!

How far do you want me to take this @DocMoebiuz? Just renaming the .cpp and .h file is easy, but there are also #define, class names, variable names, etc.

I chose not to do the rename originally, both here and on the desktop, due to the amount of code churn it would cause.

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Firmware for this pull request:
Firmware.zip

@github-actions
Copy link

github-actions bot commented Jan 5, 2022

Firmware for this pull request:
Firmware.zip

@neilenns
Copy link
Contributor Author

neilenns commented Jan 5, 2022

RAM/Flash usage after this change:

image

@DocMoebiuz DocMoebiuz merged commit b9e6244 into MobiFlight:main Jan 5, 2022
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.

Add support for input shift registers (e.g. 74HC165)
4 participants