search

esl / erlang_ale

Erlang Actor Library for Embedded -- An embedded framework from Erlang Solutions
Apache License 2.0
207 stars 65 forks source link

Add driver supervisor, MCP23x17 and MCP7940n support #27

Closed ethrbh closed 9 years ago

ethrbh commented 9 years ago

hello,

First of all, I would like to apologize to this huge changes, but...I hope you can verify and merge it to the main branch.

What changes I made so far:

Please let me know if something is not clear, or I did something wrong.

thanks for your help, /Robi

knewter commented 9 years ago

This is an amazingly detailed pull request and I'm supremely grateful for it. Would it be possible for you to resolve the merge conflicts? Also I'd love to hear @fhunleth's take on it, but I'm :+1:

ethrbh commented 9 years ago

hello Josh,

No problem, I try solve the conflicts, but please help a bit. Is it possible to see these conflict in the Github web page, or these can be seen only if I try merge these two repos in my local workstation?

Unfortunately I don't know well Github interface yet, but I try learn of course.

thanks for your help, /Robi

knewter commented 9 years ago

can you give me push rights to your erlang_ale repo and I'll just push the merge conflict resolution? Already did it here, just want to wait on @fhunleth signing off before I click the button - that way we can just click 'merge' in here later :)

ethrbh commented 9 years ago

hello Josh,

I have added you as collaborator to my erlang_ale/all_in_for_pull_request branch.

thanks for your help, /Robi

knewter commented 9 years ago

Alright, I pushed the merge conflict resolution up. Can you fetch and confirm I didn't somehow goof it up? (i.e. nothing seems to have gone wonky in that last commit from me)

ethrbh commented 9 years ago

hello,, I have fixed the merge conflict in erlang_ale_sup.erl, and now it looks able to merge.

thanks for you help, /Robi

fhunleth commented 9 years ago

Hi guys - This is a really large PR! I realize that I'm guilty of sending them on this project, but I think that this one will take me some time to review. It would be nice if the commits could be rebased so that common changes would go together and to merge commits like "fix typo". Right now, it's really hard to review each individual commit, since later commits fix issues. I'll be looking at all combined changes, but that's not ideal either since lots of things are going on. For example, there are changes to the copyright headers. They seem benign, but if there was a problem with them, it would be nice if they were in a separate PR.

Overall, I like the documentation improvements. I think that the support for other devices should go somewhere else, since I don't know how we can maintain them. The project is already difficult to maintain due to the lack of unit tests, and not having the hardware to verify that they work will make that code very hard. I'm open to ideas, since having interface code for a variety of chips does sound very useful. Regarding the supervision addition, I need to look at your updates to form an opinion.

ethrbh commented 9 years ago

hello Frank,

So what next can I do? Should I do separated PRs for the followings for example?

thanks for your help, /Robi

fhunleth commented 9 years ago

You don't need to make separate PRs yet, but merges are all or nothing, so if we decide against one thing, then nothing will get it in. I think that it's more important to rebase like changes together. That will also make it easier to separate out PRs if that's necessary, but I still think that's a separate step.

I'd really be interested in brainstorming what to do about unit tests and regression tests on this project. I think that this is erlang_ale's biggest issue when it comes to maintainability. It addresses your commits since you're adding features that I probably won't use much (mostly since I don't have the right hardware).

ethrbh commented 9 years ago

hello Frank,

Please do not get me wrong, but last week when we have started a discussion about handler and chip support within PR #22, nothing was decided, but that is true that this way was OK for you. And it is also true that I forgot to split my ale solution, because I just focused on the supervisor and chip support to be completed, let say in time, because my "original" project requires that.

I also agree with you that review all my changes I did within this PR is quite lot, and huge work for you, and that is why I said that I can split to few PR, for better understanding, and for easier review.

For CT, I agree with you, that this is a bottleneck of this project, but I think it is hard to write TCs without decide what HW should use for that, and what kind of embedded systems should support, Raspberry Pi, ect. Sorry, I'm wrong, I saw that Erlang Embedded Demo Board + Pi is mentioned in the README.

One possible way can be, that the contributor who is going to be merge a PR, should run the existing CT -few Test Cases should be ready at least- and merge the PR if all TCs are passed, reject the PR or start discussion with the PR owner if CT fails. But the HW what should use for CT should be decide. I am open to start write test cases for the existing features, so not for my changes, because these are not part of the project yet.

Finally, I really would like to apologize that I miss understand you about "moving" chip support part into a separated project. If this is a decision from all of you, tomorrow I can start my day with moving stuff.

thanks for your help, /Robi

fhunleth commented 9 years ago

I'm being wishy-washy about chip support, since I don't know the right answer and I feel like if we start supporting chips that we should have a plan on how to maintain them. You have a good point about the Erlang Embedded Demo Board. The Erlang Demo Board came out of some history with the project. Since it's pretty hard to acquire one now, I don't think it's a good reference platform, but your point is fair.

Without a CT solution, my opinion is that this project should be kept as simple as possible and delegate chip support to other projects. My experience with open-source projects is that contributors come and go, and if some code they wrote fails after they've gone, they won't respond to emails.

The Node.JS Johnny Five project may have some ideas for us. They mock out the hardware so that it's possible to write unit tests for chip support. See https://github.com/rwaldron/johnny-five. Unfortunately, they're using the Firmata interface to Arduinos and other boards, so they're stuff isn't reusable for us.

I've been thinking about a "mockproc" project where we have a mock /proc filesystem that can simulate chips without requiring real hardware. Erlang/ALE is really only a wrapper on a few interfaces in /proc, so it seems like it might not be too hard. We'd need to use a custom FUSE filesystem to simulate /proc, but it doesn't need to be particularly fast or featureful. Alternately, we could simulate everything at the Erlang level, but given how much C code is critical to Erlang/ALE working, it seems wrong to leave it out.

Anyway, no apologies are necessary. Before you make a lot of PRs, let's figure out how to organize what you have here and figure out the test strategy. My concerns are with maintenance, and I was concerned about it before you ever sent your PR. You're just unlucky that your PR came before this was figured out. I really appreciate all the work that you're putting into Erlang/ALE, so thank you for hanging in there as this is figured out. (Or Josh can go ahead and merge and we can figure it out in master since I'm technically not a maintainer. :smile:)

ethrbh commented 9 years ago

hello Frank,

Reading through your last comment, I think, I understand your view about Erlang/ALE should keep as simple as possible. This is a fair view, but this also means to me that all improvements, changes what are not really related to the Erlang/ALE, but adding more features should/must add into a separated project. This is fine for me, I can agree with you, and I think it is not too complicate to leave with this. With this way Erlang/ALE can be simple, and may re-usable for others without any "limitations", "requirements", ect.

So, if it is fine for you and Josh, I can close this PR, for leave Erlang/ALE as it is today, as a simple, easy maintainable project, and start move all my changes into a separated project.

Regarding to CT, I would vote to try "simulate" the HW anyway, and build the CT based on that. Make a decision to have same HW platform for all who can have access to merge PRs is easy, but maybe not easy to "buy, build-up" the HW itself. With the simulation, the Continuously Integration (CI) of Erlang/ALE will be much easier. In the other hand, simulate the HW and make test based on this will not cover a full test, real HW is needed anyway. And here the question is, who and when will test the project with real HW.

thanks for your help, /Robi

fhunleth commented 9 years ago

Hi Robi,

Don't close this PR! :) I didn't mean to scare you away by that last email. I tend to work slowly, so don't think that I'm not interested since I had comments to think about.

Let's make our way through the changes one by one. The documentation change is a no brainer to integrate, so let's start with that. The good news is that I just received commit privileges to this repo. The bad news is that I'm leaving for Boston for a conference this afternoon. Let me see if I can rebase your changes to tease out the documentation changes and then lets get them in (not sure how much time I'll have this week, though). If I move too slowly on the other changes, feel free to start another project. I still think that's the right way to go for chip support. We'll want a reference to your project here, though. But please don't close this PR yet, since you've done a lot of work and I don't want to lose it.

ethrbh commented 9 years ago

hello Frank,

Ok, I will not close the PR. It is feel free to review, and merge :-) And I really do not want runaway and leave Erlang/ALE. No, I will not do that. But today I have started to continue my other project which one uses my forked Erlang/ALE. So I can use the forked ALE as dependency until we will not have "official" decision about what possible to include and merge into ALE, and which ones are not "recommended".

What is good that I have done lots of changes, and those are working on my prototype board (this is NOT the Erlang Demo Board, but designed by me :-) )

So, right know, I can work, and of course I still would like to improve my solution, regarding to the supervisor and chip support area, but NOT pushed those into that branch what I requested to pull into the main. I started a new branch already for this.

thanks for your help, /Robi

fhunleth commented 9 years ago

Robi - See PR #28. I pulled out your "make docs" updates and made a PR with just them. Let me know if I missed anything, and I'll merge.

ethrbh commented 9 years ago

hello Frank,

All looks fine. So you go ahead to merge it.

thanks, /Robi

fhunleth commented 9 years ago

Cool - I'll be slowly going through your other changes as I get a chance.

ethrbh commented 9 years ago

hello Frank,

Just FYI, that I have another branch, called "all_in", what is based on that branch what you are review. So I have made lots of changes, bug fix, improvement mostly for MCP23x17, because I have found faults surround GPIO interrupt handing/configuration.

Because I have another project where this modified Erlang/ALE is used, I think I will have more updates in the next few days.

I also added example for MCP23x17 GPIO interrupt handling but this is not documented yet.

br, /Robi

fhunleth commented 9 years ago

Could you make a pull request that only has the GPIO interrupt handling fixes and base it off of the current master?

ethrbh commented 9 years ago

hello Frank,

What do you mean the current master? Master in esl/erlang_ale or master of my forked version? Sorry if I asked stupid things. Btw, the changes I made in MCP23x17 since this pull request looks too much:

I am so sorry, but these changes are really needed, that is why I shared this with you, but I know I made a mistake that is why I did not recognized these faults before issued the PR :-( Mea culpa.

So maybe better would be to merge these news into that branch (all_in_for_pull_req) what belongs to this PR.

What do you think about it?

thanks, /Robi

fhunleth commented 9 years ago

Master is esl/erlang_ale. Please send the changes one at a time. It's really hard to review if you send them in batches.

ethrbh commented 9 years ago

hello Frank,

I have an "old" problem with github. It is not trivial to make PR/commit. I mean if I have a branch and that I would issue a PR, I can do it based on the latest commit of the branch. I think this is what You do not want to see right now, because since my last PR, I have made lots of changes, but you want see these changes lets say commit-by-commit.

I "asked" google how to do this in github, and I found this page, http://stackoverflow.com/questions/5256021/send-a-pull-request-on-github-for-only-latest-commit, where describes how possible to issue a PR for NOT on the latest commit but some other. I think I can use this technique for make PRs per commit. What do you think, should it work?

thanks for your help, /Robi

fhunleth commented 9 years ago

Yes - cherry-picking your last commit and then applying it to a branch off upstream master should work.

ethrbh commented 9 years ago

hello Frank,

Maybe I did something wrong, but I think the cherry-pick will not work. What I did is to make a branch based on the current master, and cherry-pick one of my commit into. The commit what I picked does not contains only one file what I fixed, thus the "picked" repo looks like the maseter + my file, and all the others what I made already is missing. Thus I would have two proposals for the solution: 1: move all my changes into a separated project, as you have mentioned before 2: make a new branch in my forked repo, and push my changes into that from the beginning, but not all what I already have but few of them, and make a PR. Than you can review this PR, and merge if all looks fine. Once you have merged the changes into the master, I can add more commits to my branch and make a new PR, and so on until all my commits have been merged.

What do you think about it?

thanks for your help, /Robi

ethrbh commented 9 years ago

hello,

I have made a project https://github.com/ethrbh/erlang_ale_extension, what contains my changes for different chips support. My plan is to add more-more chips and use erlang_ale as a dependency project, thus as you have mentioned, erlang_ale can be kept es simple as possible.

So I will close this pull request, and will continue work on the erlang_ale_extension.

br, /Robi