Open TheLancashireman opened 3 years ago
There has been some discussion about supporting pcf2119x devices in the past. Here is one such discussion: https://forum.arduino.cc/t/arduino-with-i2c-lcd-bthq-21605av-with-chipset-pcf2119/427013 These devices have several issues that makes it a difficult to fit into a common hd44780 platform interface with font character set being one of them.
I think it would be better to have a discussion on what is really needed to support this type of device to see how it could be best implemented before trying to actually implement something. I am very leery about putting something in the hd44780 class to support this mapping given it adds overhead to everything even when not being used. My thinking is that the device is so out there with issues that any device specific things needed to make it "work" would be best done privately in its own i/o class rather than modify the hd44780 class to support a mapping capability. Particularly since even after adding the mapping capability it still may not offer full font capabilities. It also needs to resolve the clear() issue. This can't be done by creating a wrapper around the hd44780_I2Clcd i/o class. The way it could do all of this like any needed device specific initialization, character mapping, or clear() handling would be kind of like what the hd44780_NTCU165ECPB i/o class had to do. i.e. snoop instructions, maintain state, and sometimes do special things.
I think the Arduino forum under the displays section would be a good place for this discussion to start, to get the high level details worked out.
From my initial look, as is, this code cannot be pulled in. There are several issues.
mapping code cannot be done/called in _write() _write() is for sending raw data bytes with no modification/manipulation to the display. Since this code maps in _write() it will break things like custom characters that need to send raw data bytes to the display to set up CGRAM since it maps bytes that are being sent . If you were to run the LCDCustomChars example, it would have issues as would any sketch that used custom characters. If a mapping capability were to be added to hd44780 (which I don't think it should be), but if it were, it would need to be done in write() vs _write()
Supporting mapping in hd44780 puts a penalty on all the other i/o classes. This implementation adds overhead (code size, data size, and runtime) to all the other i/o class, for a mapping functionality that probably will never be used/needed since so far no other i/o class has needed this type of mapping functionality.
adding clear_row() does not solve the clear() issue The API clear() function needs to work. Adding a clear_row() doesn't resolve the issue. i.e. a sketch written to use the LiquidCrystal or LCD 1.0 API should "just work" with this hardware after changing the header file and lcd object declaration. There should be no need to change any actual sketch code.
adds a generic public API function to a specific i/o class A public API function like clear_row() is not specific to an i/o class and therefore should not be added to a specific i/o class. i.e. it is not needed to add pcf2119x support or its character font mapping. I am not interested in adding any non i/o class related public API functions into an i/o class as that breaks having a single common public API that is available across all devices which is one of the main features/goals of the hd44780 library. While at some point there might be a need to add public i/o class specific API functions, so far I have also been able to avoid this. In terms of an API function to clear a row (not as a clear() substitute) that is something that would likely be part of terminal display capability mode that I have considered for quite some time, but have not yet implemented. I have been weighing doing it in hd44780 vs creating a wrapper templated class that sits on top of hd44780 so that those that don't want/need the added functionality don't have any added resources taken up.
adds an i/o class with no examples The i/o class is incomplete. All i/o classes in the hd44780 library include a full set of examples and documentation to demonstrate the use of the i/o class. This includes the wrapper sketches for the hd44780examples, readme files, as well as the i/o class specific examples. Also, at a minimum the HelloWorld example, should include things like pointers to datasheets as well as notes and warnings about use of the devices. And for this device it should include information on the character mapping and its limitations.
I put the call to remap() in the write() function, not _write(). As far as I can tell, that function is only used when writing to the display RAM. I agree, it's a small overhead for all other display types, but I can't think of any other way to squeeze it into the existing classes. It might be possible to move the remap() call to the iowrite() function in hd44780_I2Clcd - at least then it would only be an overhead for pcf2119x devices. However, I was concerned that putting it there would break the CGRAM functionality.
I would have liked to override an existing function without modifying the base classes, but there doesn't seem to be a suitable one. Unfortunately, my C++ skills are seriously outdated - I haven't used C++ seriously for > 20 years - so maybe I'm missing something...
The clear_row() is a temporary workaround that's useful in my application. I'd be happy to move it out of the pcf2119r class and back into my application if you would prefer. That's where it was originally.
I put the call to remap() in the write() function, not _write().
Ah, yes. I see that now. This is a big OOPs for me.
For some crazy reason I had incorrectly thought that was in write(), I have no clue why. Doh!
Anyway, I'm still not going to accept this pull request as there are still other outstanding issues and it is incomplete. There is the issue of clear() not working, potential Function Set instruction issues, the clear_row() API function, and all the missing examples and readme files. And how to handle, if at all, some of the missing glyphs like [ \ ] ^ { | } ~ Some of those seem important for compatibility with existing sketches that may use menus.
Like I said before, one way to make it work is to put all the code in to its own i/o class rather than attempt to wrap the hd44780_I2Clcd i/o class. Then do any needed special stuff using techniques kind of like what the hd44780_NTCU165ECPB i/o class had to do. i.e. snoop LCD instructions as they are handed down to iowrite(), maintain states if needed, and sometimes do special things when necessary based on states and the hd44780 LCD instruction being processed in iowrite(). This would keep all the code for this in the i/o class and not affect any other class including the base class. It would also allow clear() to work which it sounds like might eliminate the need for clear_row() ?
I would prefer to move further discussion of pcf2119x support outside this pull request since there are still several outstanding things that need to resolved before any code can be written, much less pulled in.
I would move the discussion to either an hd44780 issue or as a topic on the Arduino Forum under displays.
This change modifies the base class slightly to add a virtual remap function with a default identity map. The new class pcf2119r overrides the default with a mapping that attempts to make most of the ASCII set available. If also adds a clear_row() method to work around the problems with the build-in clear commands.