Skip to content

Added support for RC5 extended #522

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

Closed
wants to merge 8 commits into from
Closed

Conversation

LarsWH
Copy link

@LarsWH LarsWH commented Sep 14, 2017

Hi,

I added support for 'RC5 extended' in order to make this project: https://www.instructables.com/id/Remote-Control-for-Lava-mMotion-Swing-Mounting-Bra/

The modification is working nicely in this single application. It has not been tested elsewhere.

(I am new to Git and this is my first pull request ever. So please excuse me if I have messed something up)

Kind regards,
Lars

@z3t0
Copy link
Collaborator

z3t0 commented Sep 19, 2017

Everything seems to have been done excellently. Are you sure it's the first time you have used git 😄

For the sake of being verbose might I request you add a small description of your changes to changelog.md and then push that commit upstream? I will merge once that has been added.

@LarsWH
Copy link
Author

LarsWH commented Sep 19, 2017

Hi Rafi,
I have added a one-liner to changelog.md and pushed it.
I have only been using Git as a single-user thing, and most of the time only locally on my computer. This is indeed my first time to make a pull-request. Fingers crossed...

@AnalysIR
Copy link
Contributor

@z3t0
some comment based on a very short/incomplete review

ir_NEC.cpp
delay(200);  // Added by LWH

That is a significant delay to introduce before sending a signal. IS there a good reason for it?

irRC5_RC6.cpp
#define RC5_T1             830 //LWH originally: 889

The original value is correct (830 is not RC5)

void disableIROut ( ) ;

Not sure of the rationale for this? What is it?

Finally, is there a case to be made that sendRC6ext could make use of sendRC5 to achieve the same result with less flash memory used up. (Just generate the parameters required by sendRC5 in sendRC6ext

@LarsWH
Copy link
Author

LarsWH commented Sep 19, 2017

Are those changes really in the head version? Then I really did mess up :-(
The head version should ONLY contain changes to ir_RC5_RC6.cpp and IRremote.h.
The other things comes from experiments (an Arduino that is receiving from a Sony remote control and passing on signals to a Nec and RC5 devices).
It is a bit late here in Denmark, but tomorrow I will try to figure out how I messed up my head revision.
[Edit] I did not look into RC6 at all, so I do not know if any reuse is possible. I only needed to use RC5 and RC5ext for my project.

@LarsWH
Copy link
Author

LarsWH commented Sep 20, 2017

@AnalysIR @z3t0 I have verified that the changes from Rafi’s latest version (741d9e, September 10) to my latest version (ddac72, September 19, see screen dump) does not contain any of the experimental stuff you listed. Can you please double check you are seeing the same?

I did joggle around with a few branches, doing experiments for that other project I mentioned, but those hacks should be all gone now. Those experimental branches/commits should really not have been visible to others, but obviously I have not yet fully understood the Git flow. My apologies.

image

@AnalysIR
Copy link
Contributor

FI: I just clicked on the links earlier in this thread, to find the code...otherwise I'll leave it to @z3t0 to figure it all out :)

New changes since you last viewed
LarsWH added some commits 8 days ago

@z3t0
Copy link
Collaborator

z3t0 commented Sep 24, 2017

It all looks good on my side. Will merge after bumping the version number

z3t0 added a commit that referenced this pull request Sep 24, 2017
Credits: https://github.com/LarsWH
Excerpt from PR:
```
Hi,

I added support for 'RC5 extended' in order to make this project:
https://www.instructables.com/id/Remote-Control-for-Lava-mMotion-Swing-Mounting-Bra/

The modification is working nicely in this single application. It has
not been tested elsewhere.

(I am new to Git and this is my first pull request ever. So please
excuse me if I have messed something up)

Kind regards,
Lars
```

Merged #522
@z3t0
Copy link
Collaborator

z3t0 commented Sep 24, 2017

Awaiting for the checks to finish on the other branch. Will merge after.

@z3t0 z3t0 mentioned this pull request Sep 24, 2017
@z3t0
Copy link
Collaborator

z3t0 commented Sep 24, 2017

@LarsWH This has just been merged onto beta. Will mark it as such and close this PR once it makes it onto a release.

@z3t0 z3t0 added this to the 2.5.1 milestone Sep 24, 2017
@LarsWH
Copy link
Author

LarsWH commented Sep 24, 2017

@z3t0 Thank you for accepting my enhancements into your excellent library :-)

@ArminJo
Copy link
Collaborator

ArminJo commented Aug 2, 2020

Included in 2.5.0

@ArminJo ArminJo closed this Aug 2, 2020
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.

4 participants