Skip to content

Adding more authorized contributors #525

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
z3t0 opened this issue Sep 21, 2017 · 36 comments
Closed

Adding more authorized contributors #525

z3t0 opened this issue Sep 21, 2017 · 36 comments
Labels
Keep Keep issue

Comments

@z3t0
Copy link
Collaborator

z3t0 commented Sep 21, 2017

Hi all!

I greatly appreciate all the effort put into this project by members at all levels, including programmers, testers and users who provide valuable feedback. Clearly, I have fallen behind quite a bit in the development and PR merging phases. However, this is just how open source projects tend to go as everyone here is a volunteer working on their own time and for free.

I would like to open up the floor to promote some of the core contributors to being authorized to directly accept PRs and add commits to the repository. This way the project can continue to grow at a faster pace and will have more than just one contributor involved in the final release process.

The only requirement is that you have spent a reasonable amount of time being visible in this project. Either by supporting other users, submitting PRs, or testing the library.

@ArminJo
Copy link
Collaborator

ArminJo commented May 23, 2020

Hi Rafi,
I agree with you, that the library is more or less complete.
Maybe porting to new architectures and adapting to new Arduino versions if ever.

The main effort must be put to clean up all the loose ends and tag the remaining issues and PR, to make it transparent at one look, what the state of the repo is.

So I can offer to help maintaining this repo and try to give it a "fresh look" 😀 to keep its value for the Arduino IR community.

@z3t0
Copy link
Collaborator Author

z3t0 commented May 24, 2020

Great! I've added you as a contributor. Please let me know if you are able to do things like close issue/change labels etc.

@ArminJo
Copy link
Collaborator

ArminJo commented May 24, 2020

Thanks.
I am currently trying to get an overview.
In this context I have 2 questions.

  1. Which branches contain valuable code extensions?
  2. Whats about the 3 projects?

@ArminJo
Copy link
Collaborator

ArminJo commented May 24, 2020

And another question.
I am not able to push to master branch since it is protected.
remote: error: GH006: Protected branch update failed for refs/heads/master.
I removed 2 Compiler warnings 😀 .

I personally use mainly the master branch, if CI (by Github Actions or Travis) is installed.
Or do you wish to create a new branch, which I personally would like to avoid, since closing issues by referring to a branch, which is eventually deleted later, seems quite confusing for me.

@z3t0
Copy link
Collaborator Author

z3t0 commented May 25, 2020

My responses below, apologies for the poor formatting! I wanted to get the info out quickly haha and thought it should be fine to just bullet point them.

  • Most of the branches aren't actually useful.
  • The master branch is the stable code and ideally the latest release of the library (See Releases Tab)
  • The dev branch is where we do development and merge PRs into for releases
  • I have gone ahead and cleaned up the branches to make this a bit clearer
  • The "head" branch seems to have some useful code that maybe should be merged in. See renamed "boarddefs.h" to "IRremoteBoardDefs.h" #405 for more info
  • The salocinx-master branch (see Salocinx master #415 useful but should not be merged at the moment. As previously stated I have decided we should not be adding more protocols to the library. However I am starting to rethink this. A lot of the users for this library will find it difficult to implement their own protocols, maybe we should keep a list of supported protocols and we should accept community support in extending it, perhaps you could create an issue where we can discuss this and come up with a design?
  • Similar with beta branch; Merge beta to master #653 it has a protocol extension
  • The rest of the branches look like they were half implementations of features or considerations for PRs and changes that never moved forward, I'd say ignore them for now and we can come back to it once other parts of the project have been cleaned up.
  • The 3 Projects aren't really anything haha, they were me trying to either rewrite the library or add more documentation. I wanted to add issues to each of the projects for progress tracking. I think we can safely delete them all and create a new one in place to "freshen" the library, feel free to do that if you'd like!
  • Master is locked because ideally I don't want us to be pushing to master, even as maintainers. I want master to be only for releases. It should always be stable and tested.

Also, we do have TravisCI but testing is quite poor and I'm not even sure if the tests still work!

I think we can use the dev branch similar to how you describe the master branch in your workflow?

Also, in all of this just keep in mind this is how things are currently being done, and by no means does that mean this is how things should be done or has to be done. I strongly believe in developing software in the open and with community support and involvement. So if any of my choices don't make sense or are just simply bad choices then I invite further discussion and would love to hear alternatives. Even if you find something and are like "hmmm this seems like a bad idea" but don't have an immediate solution, then please still comment because at least we can identify issues :)

Thanks for joining the project, It's going to be fun collaborating.

@ArminJo
Copy link
Collaborator

ArminJo commented May 25, 2020

Hi Rafi,
thanks a lot for the detailed answer!

According to adding new protocols, my opinion is, that this is a community driven thing, and times are changing, so why not include new protocols like #690 to avoid everyone to scan all the forks, if it is implemented in any of these.
The same holds for new architectures. If one has a good solution, why keep it away from the Arduino community.
I have a lot of different boards at home and therefore am able to test portings.

Regarding the master branch, I know this discussion since 30 years. There is no right or false, it is merely a matter of taste.
My argument is, that most of the users are not aware of a dev branch and if I fix an issue, I like to have a reference to master. And we have the releases for keeping a frozen state, so why (mis?-)use the master branch for the same purpose?
The Arduino guys also use the master branch for development.
If you prefer the dev branch, it is OK for me, but then I would like to have a prominent note in the readme like here, that it exists and the latest fixes can be found there, so users can learn about using branches 😀 .

Ps. Please keep in mind that I am from Germany, so my english may "sound" strange.

@ArminJo
Copy link
Collaborator

ArminJo commented May 25, 2020

I am a bit lost.
The dev branch is 46 commits behind master. 👎

@bengtmartensson
Copy link
Contributor

bengtmartensson commented May 25, 2020

I would welcome if this project starts moving again. Some thoughts:

  • There is another "dev" branch, called "beta" (which is a really bad name that has caused confusion in the past). I suggest merging those two.
  • One of the design problems of the current library is the way new protocols are added. To configure for a particular application, you edit the library source files... not exactly the library idea. I think that adding additional protocol will just make things even more cluttered. Suggestion: create a branch "newprotocols", and add the new protocol contributions to that branch, at least for now. Or possibly finding a clean way to make the addition, and create a sibling project "IRremote-protocols"? Also note that I have a way of automatically generating protocol code #536.
  • This document is often considered a good best-practice for using branches.

More later!

Ps. Please keep in mind that I am from Germany, so my english may "sound" strange.

Überhaupt kein Problem!

@ArminJo
Copy link
Collaborator

ArminJo commented May 25, 2020

* [This document](https://nvie.com/posts/a-successful-git-branching-model/) is often considered a good best-practice for using branches.

Thanks a lot for this valuable link!!! I came from SVN and are still accustoming with Git...

But one remark. All these concepts are targeting on teams working simultaneous on one project. and rely on everyone to know the rules and semantics of the branches.
Thats why I personally prefer the KISS one trunk and local development strategy if most of the time only one developer is in charge.

@z3t0
Copy link
Collaborator Author

z3t0 commented May 26, 2020

Loving this discussion! My thoughts below.

ArminJo: According to adding new protocols, my opinion is, that this is a community driven thing, and times are changing, so why not include new protocols like #690 to avoid everyone to scan all the forks, if it is implemented in any of these.
Bengt: Or possibly finding a clean way to make the addition, and create a sibling project "IRremote-protocols"? Also note that I have a way of automatically generating protocol code #536.

I would like to come up with a design that makes it easy to add these protocols. And also that the end user shouldn't have to do much work. Not sure yet what the design could be. Maybe Protocols become sketches instead? That users can copy around, but this may be too much work for them.
Maybe a second library as suggested by Bengt.
Maybe we just keep piling protocols into the source code and add more documentation around using the defines and ifdefs. Thoughts?

ArminJo:
I have a lot of different boards at home and therefore am able to test portings.

That's great to hear! One of the main reasons I couldn't support all the boards the community wanted was a lack of maintainers who could test. Now we don't have to worry about that.

ArminJo: The dev branch is 46 commits behind master. -1
Bengt: There is another "dev" branch, called "beta" (which is a really bad name that has caused confusion in the past). I suggest merging those two.

Yes the branching so far has been a bit wonky. Likely we want to do a beta+dev merge and then merge master into dev and work from dev. More below.

ArminJo: But one remark. All these concepts are targeting on teams working simultaneous on one project. and rely on everyone to know the rules and semantics of the branches.
Thats why I personally prefer the KISS one trunk and local development strategy if most of the time only one developer is in charge.

Yes that actually makes a lot of sense. I guess my concerns are:

  1. Tags are less "simple" than branches. In that less people actually know how to use them. Our target audience is Arduino developers that just want to get things done, so I like isolating users from the dev changes by using a dev branch
  2. Similar to above, it is easier for users to know that master is stable
  3. Many tutorials referring to this library were created before the Releases feature was either used by us or even existed on Github. They all tell the user to download master :( ... This makes me hesitant to make master a trunk
  4. Not everyone looks at releases.

I guess I'm really trying to lower the barrier and remove any potential obstacles, such as users accidentally using dev code. I think from the development side it makes perfect sense for trunk-based and I'm starting to lean towards that a lot more on my private projects. However, in this case I am more for optimizing for the users and keeping ithem safe. Also, we do have a README but not very many people read it.. Its just a reality haha. That's one reason I'm inclined to keep master stable because of the many tutorials that ask users to download master.

Looking forward to hearing thoughts!

P.S Your English is great! No disclaimers needed haha

@ArminJo
Copy link
Collaborator

ArminJo commented May 26, 2020

OK, I think I begin to understand you.
Let me explain how I work.

I develop a feature locally until it works and then make a few compile checks with Sloeber.
Sloeber
Then I commit it, which in turn triggers the GitHub build of all examples / board combinations looking for missing libraries or compile errors.
This could be improved by commiting it on the dev branch. But anyway, after 10 minutes the compile errors (wrong #ifdefs mostly) are fixed and the master is 100% stable as before.
Keeping the result only in dev brings no benefit, since here nobody will download dev and test it in contrast to bigger projects, where testing is performed.

My first goal is to support the plain Arduino user, which looks first in the Arduino Library Manager (our Releases) or second, downloads the master, where he expect the latest bug fixes.

And for Issues I like to patch the master and tell to download and test it, before I build a new release (for Arduino).
I try do do it KISS.
But I agree, that this is not applicable to bigger teams.

For Pull Request it maybe a benfit to use the dev branch so we should advise collaborators to target pull requests to the dev branch?

And I hate stale branches like experimental or feature_xyz, where, after 2 years, nobody knows what is contained in there.

Maybe we just keep piling protocols into the source code and add more documentation around using the defines and ifdefs. Thoughts?

A possible solution for supporting multiple protocols is to set symbols for conditional compiling like -DUSE_DENON -DUSE_NEC but since this is not available for Arduino the workaround can be to put it at the beginning of the source and then including the c sources, which is a no go for every professional c programmer, I know (but it works for Arduino!).

#define USE_DENON
#define USE_NEC 
#include #include <IRremote.cpp.h>

Also, we do have a README but not very many people read it..

I try to keep attention to the readme by putting pictures in it, but it's just a guess.

BTW, If we overhaul this library what's about the library 1.5 specification? It makes the readme more prominent!

@bengtmartensson
Copy link
Contributor

what's about the library 1.5 specification?

IIRC, the "beta" branch goes a long way here. And, while on it, use the mechanisms of the library specification to not export IRremoteInt.h, differently put prohibit user's sketches from

 #include <IRremoteInt.h>

(There is a reason for the name, at least there should be...)

@ArminJo
Copy link
Collaborator

ArminJo commented May 26, 2020

Who merges the two branches dev and beta?
Should I do this?
Or better one of the creators of this branches?
Regards
Armin

@z3t0
Copy link
Collaborator Author

z3t0 commented May 27, 2020

I am starting to be convinced that we should use a trunk (master) and not a dev branch. If we can reach a consensus then let's do that.

Who merges the two branches dev and beta?

If you can create a PR to merge beta into dev (with any necessary changes) then the three of us can go over it and merge it into dev.

I think once we have two branches, ie. dev and master then we can revampt and clean dev. Once dev is in a good state then we can merge dev into master and from then on just do our work on master.

My only concern with using only master is that the code doesn't get reviewed. This may just be my old-style thinking.. But ideally all changes to the library go through a second pair of eyes. Even if the change were to come from me, I'd sleep better knowing that someone else had looked it over before it went into master.

On the one hand I want changes to go into master quickly and to be tested and landed to users. On the other hand I dont know if code that hasn't been reviewed should go into master.

What if we do this.

  1. Create a PR targeting master from a feature branch
  2. Make changes
  3. Ask for a review and then merge

I think this is what feature branches are?

BTW, If we overhaul this library what's about the library 1.5 specification? It makes the readme more prominent!

Yep! And if beta has that then lets use it.

Anything we can do to improve the library makes sense for us to do.

IIRC, the "beta" branch goes a long way here. And, while on it, use the mechanisms of the library specification to not export IRremoteInt.h, differently put prohibit user's sketches from

Agreed

A possible solution for supporting multiple protocols is to set symbols for conditional compiling like -DUSE_DENON -DUSE_NEC but since this is not available for Arduino the workaround can be to put it at the beginning of the source and then including the c sources, which is a no go for every professional c programmer, I know (but it works for Arduino!).

I think that's an option, Maybe once we have done the initial cleanup of the library we can revisit this with a few designs and then choose and implement one. We can safely disregard "professional c" restrictions that don't apply to Arduino haha :)

@z3t0
Copy link
Collaborator Author

z3t0 commented May 27, 2020

Also forgot to comment on this

since here nobody will download dev and test it in contrast to bigger projects, where testing is performed.

I think you're spot on with this. We need to "ship" more frequently so that we get user testing

@ArminJo
Copy link
Collaborator

ArminJo commented May 27, 2020

Hi Rafi,
thanks for your comments!

I had a closer look at the current state of dev and beta and the conclusion is:
Forget dev https://github.com/z3t0/Arduino-IRremote/compare/dev, use beta until merged to master and cleanup of branches.
I added CI by Github workflow and it runs seamlessly on beta 😀 .

Because beta is in a good state, I will check it the next days with extended CI checks for other platforms and test them on my boards.
Maybe I have to make some changes or include some patches from pull request to have it running on my boards.

After this I will create the PR from beta to master.
I propose to use 2.5.0 as version number (thus omitting the b from the current 2.5.0b in beta)

I would like to keep the version number 3.0.0 for the new cleanup and Include handling.

@z3t0
Copy link
Collaborator Author

z3t0 commented May 27, 2020

Hi ArminJo,

All of that sounds good to me!
If I get a chance this weekend I am going to go through issues and other development tasks that got lost in the tracker. I'll be collecting that information either into an issue or the new Project I made in this repo.

@bengtmartensson
Copy link
Contributor

Because beta is in a good state, I will check it the next days with extended CI checks for other platforms and test them on my boards.

I think that is a good choice. IMHO, more important that what branch is that a choice is made, and that the workflow is transparent and defined.

If I get a chance this weekend I am going to go through issues and other development tasks that got lost in the tracker.

Great! Feel free to ask me for reviews for individual PRs.

Protocols: before the "dust has settled", I recommend not accepting new protocols.

Hardware: I strongly recommend treating #437 with priority, and only then accepting new hardware ports on top of that.

@ArminJo
Copy link
Collaborator

ArminJo commented May 28, 2020

I started today with testing 5 examples. In order not to get confused I always add the line
Serial.println(F("START " __FILE__ " from " __DATE__));
I think this might be also useful if we get serial dumps in issues.

In other libraries I even use the line:
Serial.println(F("START " __FILE__ " from " __DATE__ "\r\nUsing library version " VERSION_SERVO_EASING));

Thats it for today.

@z3t0
Copy link
Collaborator Author

z3t0 commented Jun 1, 2020

Awesome! That's more than I've done in a few years haha.
I got preoccupied with a few other priorities this weekend so couldn't make any progress. I'm hoping to find some time next weekend. Will post updates.

@z3t0
Copy link
Collaborator Author

z3t0 commented Jun 1, 2020

Protocols: before the "dust has settled", I recommend not accepting new protocols.

Fair. I'm not sure what we should do on this so I think we can create a new discussion and revisit this once the other things are done.

Hardware: I strongly recommend treating #437 with priority, and only then accepting new hardware ports on top of that.

Will add this to the list.

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 1, 2020

The test are finished now.
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.

I will now issue the pull request (update: I seem to lack the permissions to do that, so please @z3t0 can you unlock master, or do this for me?), to have a clean state for master. I hope you will review it successful,
I hope we could merge it in the next days.
After that, we can consider to unlock master branch.

Next step is propose is to merge #437 and maybe start to look at a few open ends and close them.
@bengtmartensson Can you rename TIMER_PWM_PIN to SEND_PIN in your master? And what examples / #define's do you propose for testing #437 ?

@bengtmartensson
Copy link
Contributor

@bengtmartensson Can you rename TIMER_PWM_PIN to SEND_PIN in your master?

Sorry, I do not understand. TIMER_PWM_PIN has already been renamed to SEND_PIN (which is the better name). Or do you mean it the other way around?

And what examples / #define's do you propose for testing #437

What the PR does is to make the basic functionality of the library available to a few new hardware platforms. So it should be tested using the same tests used for other, already supported hardware, simple sending and simple receiving. Or is there anything I have missed?

Do you need me to rebase the PR?

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 2, 2020

@bengtmartensson
Here I want to pull #437 for test in my repo, and it showed the differences.
So best is to rebase it, but maybe after the merge of beta to master.
Thanks
Armin

@z3t0
Copy link
Collaborator Author

z3t0 commented Jun 3, 2020

I will now issue the pull request (update: I seem to lack the permissions to do that, so please @z3t0 can you unlock master, or do this for me?), to have a clean state for master. I hope you will review it successful,
I hope we could merge it in the next days.
After that, we can consider to unlock master branch.

Can you try now? I think I've unlocked it. Ideally though, let's hold off on making changes to master until the PR is merged in?

Also could you briefly outline the new proposed development method? I think it's trunk based? We can add the info to https://github.com/z3t0/Arduino-IRremote/blob/master/Contributing.md

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 3, 2020

@z3t0 Hi, I found the reason for not beiing able to create a PR.
It was an old PR created accidentally and labled wrong, which prevents me creating a new one.
I changed the comment of the old one now, all commits are contained 😀 .

Now its up to you to review (just have a short look, its all tested). 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.

Also could you briefly outline the new proposed development method? I think it's trunk based? We can add the info to https://github.com/z3t0/Arduino-IRremote/blob/master/Contributing.md

I can prepare a changed Contributing.md the next days.

@ArminJo
Copy link
Collaborator

ArminJo commented Jun 8, 2020

Hi Rafi,
I recently extended the contibuting.md.
Please rewiew the #653 and approve it so I can proceed.

I would like to finish this to go further before my holiday.

Thanks
Armin

@bengtmartensson
Copy link
Contributor

Here I want to pull #437 for test in my repo, and it showed the differences.
So best is to rebase it, but maybe after the merge of beta to master.

OK. So I wait until then.

@ArminJo
Copy link
Collaborator

ArminJo commented Jul 3, 2020

Hello Rafi,
I need your help. Can you please unprotect the master branch or at least allow for force pushs.
It is no fun correcting the CI errors and not beeing able to amend to a commit. It enlarges the commit history unnecessary by having some commits multiple times.
Regards Armin

@z3t0
Copy link
Collaborator Author

z3t0 commented Jul 9, 2020

Sorry I just saw this, I fixed it.

@ArminJo
Copy link
Collaborator

ArminJo commented Sep 16, 2020

@z3t0 @bengtmartensson
Hi
I put a lot of effort and code cleanup in the current version and I think it is worth a new minor version number - 2.7.0.
Any vetos?
Greetings
Armin

@z3t0
Copy link
Collaborator Author

z3t0 commented Sep 16, 2020 via email

@bengtmartensson
Copy link
Contributor

For sure (I did not know I had veto rights??!!)

Congratulations for the great work you have done the last few months :-).

@ArminJo ArminJo closed this as completed Oct 19, 2020
@ArminJo
Copy link
Collaborator

ArminJo commented Dec 12, 2020

@z3t0
Hi,
GitHub recently added discussions besides the standard issues. But this must be enabled under settings, which is not available for me, since I have only collaborator rights.
I do not think , that you want to transfer the administrator rights to mine account, so please enable the discussions under settings.
Maybe you can transfer the repo to an orgaisation e.g. to IRremote, where multiple owners are permitted.
see: https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/transferring-a-repository#transferring-a-repository-owned-by-your-user-account

Thanks and happy x-mas
Armin

@z3t0
Copy link
Collaborator Author

z3t0 commented Dec 12, 2020

Hi Armin,

Thank you for all your amazing work on this project! You've given it a new life 😄

I tried to add you as an admin but could not find the option. I've created a new organization and made you an owner. The repo is now transferred there.

I do not think , that you want to transfer the administrator rights to mine account, so please enable the discussions under settings.

Please consider yourselves an "administrator" or "owner". I believe you've done a really good job, the community certainly appreciates your contributions and you are better equipped to make decisions on the future of this project than myself!

As such, feel free to make any decisions as you see fit.

Happy x-mas! 🥳

Thanks,
Rafi

@ArminJo
Copy link
Collaborator

ArminJo commented Dec 12, 2020

Thanks Rafi
I was able to enable the discussions, so all is fine now 👍

@ArminJo ArminJo added the Keep Keep issue label Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keep Keep issue
Projects
None yet
Development

No branches or pull requests

3 participants