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

Add icp_a125 LCD used in QNAP devices #22

Merged
merged 2 commits into from
Nov 5, 2016
Merged

Conversation

sbingner
Copy link
Contributor

@sbingner sbingner commented Nov 4, 2016

This is a new device because it has input while the a106 does not. I also have no way to test an a106 so I'm afraid of breaking that driver if I made changes to it.

@haraldg
Copy link
Collaborator

haraldg commented Nov 4, 2016

Thanks for you submission. A few quick comments, as I have no time to do a full review today:

  • This PR doesn't include any Makefile code. How do you even compile this code?
  • Proper documentation for drivers also includes example code in LCDd.conf and a description in the LCDd man page. Also your user-guide chapter isn't included in the actual user-guide. See the developers guide for how to do that: http://lcdproc.sourceforge.net/docs/lcdproc-0-5-5-dev.html - especially chapter 6.

Is an a106 just an a125 without the keys? If so maybe we could drop the a106 driver after your driver is confirmed to work?

You don't need to be afraid to break the existing driver. That's what code reviews are for. (If the change is simple enough to be properly reviewable, of course.)

@sbingner
Copy link
Contributor Author

sbingner commented Nov 4, 2016

Ok wtf, a bunch of changes got left out of this pull request... not sure how that happened. It seems it only included the new files but ignored the modifications. Fixing..... 😞

@sbingner
Copy link
Contributor Author

sbingner commented Nov 4, 2016

OK, fixed that...

Yeah, it is essentially the a106 with keys and 16x2 instead of 20x2. Looking at it more closely, it might be possible to combine them since the get_key function would just have no input if there is no data coming in so long as the protocol is the same on the a106 which I think it is.

For combining code, would it be better to have width and height set in the config or have the board ID set in config? Board ID seems like it would be easier for users to figure out which one to choose...

@sbingner
Copy link
Contributor Author

sbingner commented Nov 5, 2016

OK, I tracked down the manuals for both the a106 and a125 and it turns out the a106 does support key presses also. Doesn't look like it would have hurt if it didn't either. I merged all the changes back into the icp_a106 driver and updated the documentation.

I put the documentation up on my webserver so it can be found in the future:
icp_a106 datasheet
icp_a125 datasheet

Unfortunately it also included syntax updates, should I revert all the whitespace changes so it's easier to see the actual changes?

@haraldg
Copy link
Collaborator

haraldg commented Nov 5, 2016

Good job!

As for height&width vs boardID: I'm fine either way.

Yes, please put whitespace changes and actual changes into different commits. They can be part fo the same PR of course. You can use "git diff" between local branches to verify that the final result is still the same.

I will try to ping the original author of the code to give him chance to comment on your changes.

Thanks a lot for your effor!

@haraldg
Copy link
Collaborator

haraldg commented Nov 5, 2016

Also you might want to add yourself to the list of contributors.

@sbingner sbingner force-pushed the master branch 3 times, most recently from 2b0c3d5 to 5a4fa8a Compare November 5, 2016 21:14
@sbingner
Copy link
Contributor Author

sbingner commented Nov 5, 2016

OK, broke it out into two commits and added myself to the list of contributors

@haraldg haraldg merged commit beb29e2 into lcdproc:master Nov 5, 2016
@haraldg
Copy link
Collaborator

haraldg commented Nov 5, 2016

Looks good to me and got an ACK from the original author of the driver, so I just merged this.

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.

None yet

2 participants