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.

Sign up for GitHub

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

Jump to bottom

Remove RotaryEncoder and TicksPerSecond libraries #10

Merged
merged 7 commits into from
Oct 21, 2021
Merged

Remove RotaryEncoder and TicksPerSecond libraries #10

merged 7 commits into from
Oct 21, 2021

Conversation

elral
Copy link
Collaborator

@elral elral commented Sep 6, 2021 •
edited by neilenns
Loading

w/o RotaryEncoder and TicksPerSecond libs.
Please double check FastAcceleration for different encoder types!

Fixes #2

@neilenns neilenns changed the title Elral/issue2 Remove RotaryEncoder and TicksPerSecond libraries Sep 6, 2021
MobiFlight-Admin
MobiFlight-Admin reviewed Sep 6, 2021
View reviewed changes
Copy link
Collaborator

@MobiFlight-Admin MobiFlight-Admin left a comment

Choose a reason for hiding this comment

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

This looks like a big change. I am OK if tested thoroughly and if backwards compatible.

Observations:

  • What's the benefits of having the source code from the other library now as part of our code?
  • I am realizing it is BSD license :(

Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:

  • Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
  • Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.
  • Neither the name of the copyright owners nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

@neilenns
Copy link
Contributor

neilenns commented Sep 6, 2021

  • What's the benefits of having the source code from the other library now as part of our code?

From over in the MobiFlight issue that's tracking the PlatformIO change @elral mentioned that it saves quite a bit of flash:

I moved the required functions from button and rotaryencoder libs to MFEncoder.cpp. Therefore these libs are no longer required and I do not expect changes in these libs anymore (at least which would be required for MF). Interesting side effect is that this has reduced the RAM usage by ~210byte and Flash by ~2,1kB!??

@neilenns neilenns mentioned this pull request Sep 6, 2021
Closed
@elral
Copy link
Collaborator Author

elral commented Sep 6, 2021

The original encoder lib uses the button lib, which is also RAM consuming and not required.

The original license is a topic, I thought it would be enough to add this in the top of both files (.cpp and .h).
If not I could think about alternatives.

@neilenns
Copy link
Contributor

neilenns commented Sep 8, 2021

@MobiFlight-Admin My read of the BSD license matches @elral: with the proper attribution/copyright it is ok.

Do you have any other questions/concerns with this change?

@elral elral mentioned this pull request Sep 21, 2021
Closed
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns added this to Review in progress in Initial migration Oct 8, 2021
@elral elral mentioned this pull request Oct 14, 2021
Closed
@neilenns
Copy link
Contributor

Alternative approach to this is open in PR #54

@neilenns neilenns added the Code cleanup Changes that clean up unused libraries, memory issues, etc. but do not change functionality. label Oct 14, 2021
@github-actions
Copy link

Firmware for this pull request:
Firmware.zip

@neilenns neilenns self-requested a review October 17, 2021 13:56
@neilenns neilenns added this to In progress in Code cleanup Oct 17, 2021
@neilenns neilenns removed this from Review in progress in Initial migration Oct 17, 2021
@neilenns neilenns moved this from In progress to Review in progress in Code cleanup Oct 17, 2021
MobiFlight-Admin
MobiFlight-Admin approved these changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@MobiFlight-Admin MobiFlight-Admin left a comment

Choose a reason for hiding this comment

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

We will stick with this option and evaluate the second alternative afterwards to see if there are any additional improvements.

Code cleanup automation moved this from Review in progress to Reviewer approved Oct 21, 2021
@MobiFlight-Admin MobiFlight-Admin merged commit 694813c into MobiFlight:main Oct 21, 2021
Code cleanup automation moved this from Reviewer approved to Done Oct 21, 2021
@elral elral deleted the elral/issue2 branch October 23, 2021 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MobiFlight-Admin MobiFlight-Admin MobiFlight-Admin approved these changes

@neilenns neilenns Awaiting requested review from neilenns

Assignees
No one assigned
Labels
Code cleanup Changes that clean up unused libraries, memory issues, etc. but do not change functionality.
Projects
No open projects
Code cleanup
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove usage of button and rotaryencoder libs
3 participants
@elral @neilenns @MobiFlight-Admin