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

Merge beta to master #653

Merged
merged 33 commits into from
Jun 17, 2020
Merged

Merge beta to master #653

merged 33 commits into from
Jun 17, 2020

Conversation

rBrinegar
Copy link

@rBrinegar rBrinegar commented Sep 22, 2019

Merge beta to master.
It was an old PR, which seems to be accidentally created, but we must reuse it now.

z3t0 and others added 22 commits August 10, 2017 16:57
In the newer versions of the IDE it is sufficient to simply include
the Arduino.h header file.

Fixes #463
It is difficult to maintain this over different architectures and also
not entirely that useful.

Fixes #462
* Removed some trailing spaces in IRremote.cpp.

* IRremote.cpp: fix some compiler warnings (-Wall -Wextra).
Remark: braces necessitated by the macro nature of DBG_PRINTLN.

* boarddefs.h: removed spurious #. Nuked trailing spaces.

Credits: bengtmartensson
* Add missing "paragraph" to library.properties.

* Move not-to-be-exported includes to subdirectory.
Remove #include "IRremoteInt.h" from a number of files since it
now does not work any more, and since IRremote.h #includes it already.
This resolves #464.

* Remove #include <IRremoteInt.h> from examples, too.

* Make ir_Lego_PF_BitStreamEncoder.h exported (again).

* Revoked fe24095 (paragraph) to keep 1-1 correspondance to #464.
* new file: Doxyfile; #461.

* Doxygen related documentation to two files. (Not complete.) #461.
The reason for this change is that I was informed by a community
member that the Pro Micro board does not have the default pin 13. This
causes many issues for beginners that are not familiar with the inner
workings of the library. However, I am concerned about this being a
breaking change. If there are serious issues I may just end up adding
a tutorial in the FAQ for beginners, informing them how they may
change the default pins.
This reverts commit 20a4d97, reversing
changes made to 47aadf5.
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
@ArminJo
Copy link
Collaborator

ArminJo commented Jun 1, 2020

Wrong PR

@ArminJo ArminJo requested a review from z3t0 June 3, 2020 08:30
@ArminJo ArminJo changed the title Protocol addition Merge beta to master Jun 3, 2020
@z3t0
Copy link
Collaborator

z3t0 commented Jun 9, 2020

Hi Armin! Thank you for the work on this. I've got a lot on my hands for a few weeks so I can't really commit to a specific day I'll get this done But I'll aim to work through a part of the changes every day and hopefully should have it done by mid-this week.

Copy link
Collaborator

@z3t0 z3t0 left a comment

Choose a reason for hiding this comment

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

I've looked at a few files and added some comments. Will go through more tomorrow!

.github/workflows/LibraryBuild.yml Show resolved Hide resolved
Contributing.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
examples/IRrecord/IRrecord.ino Show resolved Hide resolved
@ArminJo
Copy link
Collaborator

ArminJo commented Jun 9, 2020

Thanks a lot for your very constructive approach to the review and other discussions.
I can really appreciate it, since at the same time I have other pull requests open and its a plague to discuss every line, and to see how ideologic some people can be.
I hope you understand, I am not used to express this things in english...

@z3t0
Copy link
Collaborator

z3t0 commented Jun 10, 2020

Thanks a lot for your very constructive approach to the review and other discussions.
I can really appreciate it, since at the same time I have other pull requests open and its a plague to discuss every line, and to see how ideologic some people can be.

Haha thank you! After having been on both side of reviewing and submitting MRs I've come to conclude that committers know what they're better than the reviewers! I try to just to run through the code and see if it makes sense to me, and if not just ask some clarifying questions so I can learn!

I hope you understand, I am not used to express this things in english...

Your English is perfect 💯

Copy link
Collaborator

@z3t0 z3t0 left a comment

Choose a reason for hiding this comment

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

I've gone through the rest of the files! It was great reading through the code 😄 It's a lot cleaner and may have fixed a number of hidden bugs as well.

Once the last few comments are resolved, I am good to merge this in!

Can I get you all to the original comment on this MR once each of you are satisfied with this MR being merged in. A "voting" system just to acknowledge that we have consensus.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ArminJo
Copy link
Collaborator

ArminJo commented Jun 10, 2020

I added a few tasks to be done after the merge and before the release in the project https://github.com/z3t0/Arduino-IRremote/projects/4
I would like to start with formatting and housekeeping, to have the master in a valid and modern state (or status?).
Since most are small changes, I would like to do them direct at the master branch (except #437)

Does anybody know how to put the tasks in a sequence or how to prioritize them?

Lets go to build the next release 😀
Regards
Armin

@z3t0
Copy link
Collaborator

z3t0 commented Jun 11, 2020

I would like to start with formatting and housekeeping, to have the master in a valid and modern state (or status?).

"state" is the correct word 😄

I added a few tasks to be done after the merge and before the release in the project https://github.com/z3t0/Arduino-IRremote/projects/4

Awesome.

Does anybody know how to put the tasks in a sequence or how to prioritize them?

I don't think there's a way to do this. But we can create additional lists and improvise so that it looks like there's priority. Also since we only have a few tasks we don't really need proper organization.

Lets go to build the next release

🎉

Copy link
Contributor

@bengtmartensson bengtmartensson left a comment

Choose a reason for hiding this comment

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

braces on simple ifs (1 line blocks)

This leads to misunderstandings (is no braces on complicated ifs ok?). Better is
Braces are mandatory on all if/while/do

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 11, 2020

This leads to misunderstandings (is no braces on complicated ifs ok?). Better is
Braces are mandatory on all if/while/do

Very good Thanks 👍
and done.

@z3t0
Copy link
Collaborator

z3t0 commented Jun 15, 2020

Okay so I've just done a quick look over everything. We are good to merge!
I'll leave it in this state for a few days in case there is anything else to be raised. Else, if @bengtmartensson and @ArminJo could just make a comment or +1 this comment then I can merge it straight away.

Great work on this @ArminJo!

@bengtmartensson
Copy link
Contributor

From me, it is "go for launch" ... sorry "go for merge"! (I just saw an Apollo 11 movie ;-))

To get this three years old stuff finished would be great. Everything is surely not perfect, but it is not the end, rather it is a start.

I have not checked everything, not even that it compiles and runs, but my internal warning bell does not go off either.

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 15, 2020

For me of course it is OK to merge.

Everything is surely not perfect, but it is not the end, rather it is a start.

I totally agree 👍.
Thats my approach, 😀 see the next activities I proposed.

Does anyone know of https://github.com/crankyoldgit/IRremoteESP8266/?
And BTW, the have Doxygen, I looked at it and found it useless. IMHO. if you have it, than you should put some effort in delivering sensible content, otherwise it is useless.

@z3t0
Copy link
Collaborator

z3t0 commented Jun 16, 2020

Does anyone know of https://github.com/crankyoldgit/IRremoteESP8266/?

I recall there may have been an issue and then that lib was formed as a spinoff.

IMHO. if you have it, than you should put some effort in delivering sensible content, otherwise it is useless.

Agreed. When (if) we do add doxygen as documentation then it should be well-written. Otherwise, it's better for us to not put in the effort of making it.

Merging!

@z3t0
Copy link
Collaborator

z3t0 commented Jun 16, 2020

Ah wait nevermind, I spoke too soon! I just noticed there are resolve conflicts preventing a merge.

@ArminJo could you take a look?

@bengtmartensson
Copy link
Contributor

Does anyone know of https://github.com/crankyoldgit/IRremoteESP8266/?

Yes. First, the ESP8266 is, despite of its popularity, IMHO not a very good board for IR. I tried it for Infrared4Ardunio, but gave up -- the link explains the rationale. (The hardware independent stuff like sending with soft carrier and receiving with polling works though). However, the successor ESP32 is very nice, and has a usable API -- actually much nicer than AVR.

The project is, as the name suggests, a fork that only supports ESP8266 (and ESP32). IIRC, things they do that the current project doesn't (it was half a year since I looked at it):

  1. Interrupt driven reception, cf. #439,
  2. Support for some other protocols, in particular air conditioning,

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 16, 2020

Fantastic, the merge is in sight.
Thanks for reviewing! 👍

Ah wait nevermind, I spoke too soon! I just noticed there are resolve conflicts preventing a merge.

@ArminJo could you take a look?

I mentioned the merge conflicts here

I included the ProMicro definitions in boarddefs.h to make merging of the pull request easier. The beta version is now OK and tested for the ProMicro (and the Leonardo too 😀 ).
The IRrecord. ini has also merge conflicts, but I also included the changes from mater into the beta version, so we can just take the beta version.

and here

Who should merge the 2 conflicting files? You can do it if you want, just keep the beta files. If not, tell me, then I will do the merge.

but is was not easy to find them even for me.

Anyway, should I merge?
I just tested it again on my local fork 😀 and it worked and CI was succesful, taking the two files from beta.

@z3t0
Copy link
Collaborator

z3t0 commented Jun 17, 2020

Anyway, should I merge?

Sure 😄

@ArminJo ArminJo merged commit 1811b69 into master Jun 17, 2020
@ArminJo
Copy link
Collaborator

ArminJo commented Jun 17, 2020

Sure 😄

Done.
Additionally I commited the correct version of keywords.txt.
The dev and beta brances will be deleted at the weekend.

@bengtmartensson
Copy link
Contributor

Gr8!!

I run doxygen and put the API-documentation (just the api-doc directory renamed) available here.

This is not a commitment that I will keep it there, nor that it will be updated.

@ArminJo ArminJo deleted the beta branch June 24, 2020 21:07
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