-
Notifications
You must be signed in to change notification settings - Fork 86
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
dm140: add driver #47
base: master
Are you sure you want to change the base?
Conversation
Hm... It builds with LibreELEC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this. However clearly more work is needed before merging this (if possible at all). Please see the lcdproc developers guide for requirements of new drivers, especially copyright, documentation and coding style.
The various files included make me a bit uneasy. Where are the coming from? Shouldn't there be an external dependency instead?
I added a number of inline comments, pointing out obvious issues. But this is far from complete.
Also it seems your code doesn't acutally build. Have a look at the build logs to see what the problem is.
@@ -9,7 +9,7 @@ AC_ARG_ENABLE(drivers, | |||
[ which is a comma-separated list of drivers.] | |||
[ Possible drivers are:] | |||
[ bayrad,CFontz,CFontzPacket,curses,CwLnx,ea65,] | |||
[ EyeboxOne,futaba,g15,glcd,glcdlib,glk,hd44780,i2500vfd,] | |||
[ dm140,EyeboxOne,futaba,g15,glcd,glcdlib,glk,hd44780,i2500vfd,] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep the alphabetic sorting
if (p == NULL) | ||
{ | ||
report(RPT_CRIT, "Failed to allocate memory for PrivateData\n"); | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think RPT_CRIT is a bit thouch. Other drivers use RPT_ERR
@@ -128,6 +128,10 @@ dnl else | |||
DRIVERS="$DRIVERS debug${SO}" | |||
actdrivers=["$actdrivers debug"] | |||
;; | |||
dm140) | |||
DRIVERS="$DRIVERS dm140${SO}" | |||
actdrivers=["$actdrivers dm140"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you should ensure that all necessary dependencies are available.
int i; | ||
|
||
report(RPT_INFO, "%s called with values(x,y,c): %d, %d, %s", __FUNCTION__, x, y, buffer); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably you want debug() instead of report() here.
VFDSetString(drvthis, y, 1, p->framebuf[i]); | ||
} | ||
|
||
// Don't know what to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not very reassuring ...
@@ -0,0 +1,241 @@ | |||
/* | |||
* dm1400 vfd driver (c)2007 Henrik Larsson | |||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, what's the license of this code? And I think it is lcdproc policy to only merge code with the actual authors consent...
p->gFlags = 0; | ||
p->gDisplayMode = VFD_MODE_NONE; | ||
|
||
if ((p->framebuf = (char *) calloc(1, p->height)) == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIKS this allocation doesn't match the declaration.
My intention is only to have this merged upstream, for the benefit of everyone. I did not write the code and I would be unable to fix it. A quick search suggests that the author submitted it to lcdproc 0.5.3 (https://www.mythtv.org/wiki/Futaba). I do not know why it has not been merged, yet. lcdproc devs could maybe take this up from here? Thank you in advance. |
If you followed the link on this wiki page, it would have been obvious to you why it hasn't been merged yet: License isses. Like what I wrote an hour ago. However a driver called futaba was merged recently. I don't know if this supports the same HW. You would need to chack that yourself.
Most certainly not. |
Hello, Thank you. |
Personally I'm not interested in working on drivers, for which I don't have the HW to test them on. And it seems nobody else is really active. If dm140 is similar enough to the HW supported by the existing futaba driver, your best bet probably is to ask the author of the futaba driver if he is interested in adding support to his driver. (Or find somebody else who cares enough to do the work.) Since I feel somewhat pestered, I want to point out that drivers aren't added to lcdproc based on the popularity of the HW. Drivers are added if the HW is available easily enough (eg can be purchased new) AND somebody provides code of sufficient quality and proper licensing. |
I would really love to see support for dm140 again too. It's a sad thing to see an empty display since a few months now. |
This driver does have some issues. A portion of the screen isn't used (the first 8 columns of pixels aren't used). It has 112 columns and 16 rows of pixels total. |
i can't comment on the pixel alignment issue, but from having tinkered with it i can say that the kodi client does try to use special symbols where they are available. so if possible they ought to be added into this driver, but as @haraldg has stated there is still the license issue with this driver. |
just to add, i looked at the patch used in openelec to incorporate this driver, and while legalese may not be my strong suit, the way i read it this driver uses files written by Advanced Micro Devices, Inc. with a GNU v2 header in them, as this is a derivative work, would that not mean this is GNU V2 aswell? |
the way i read it this driver uses files written by Advanced
Micro Devices, Inc. with a GNU v2 header in them, as this is a
derivative work, would that not mean this is GNU V2 aswell?
Unfortunately it's not that easy: If you mix something that is
GPLv2 (only) with something else the result is not distributable
at all, unless the "else" is a very permissive license like BSD.
That's even true for mixing GPLv2 and GPLv3 when no special
exemptions are granted.
However I haven't looked into this specific case in detail, so
maybe there is an easy solution - but then people have failed
to point it out for many years.
OTOH writing lcdproc drivers is fairly easy - everybody with
basic C skills could rewrite the driver from scratch or from the
GPLv2 file you found. Maybe that would be less work then
investigating the situation. However it seems nobody cares
enough to do it.
…--
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or CLAM xASPBtezLNqj4cUe8MT5nZjthRSEjrRQXN
|
fair enough, I wasn't sure if it was that simple, but then licenses aren't my strong suit. but as you point out there are issues with the driver as written, even the original author has admitted in various forum threads that there are some issues with it, along with some features not working at all (volume bar) |
On 11.04.2017 12:53, James wrote:
even the original author has admitted in various forum threads that
there are some issues with it
If people know how to contact the original author, then maybe he can
be bothered to address the issues (legal and technical) in this PR.
|
I couldnt get my dm140 to work with kodi at all. I did set up xbmc-lcdproc properly but im getting nothing from the screen. |
Hello,
The dm140 driver exists as a patch to LibreELEC/OpenELEC.
It would however be profitable to merge it upstream.
Thank you for your attention and support.