Skip to content

Conversation

@jmiguelf
Copy link

Simple (based on arduino direct io) IO lib that works using a mcp23S17 to control an LCD, i'm using Adafruit_MCP23X17 lib so it should play nice with all supported ic's.
Tested with webmos D1 mini and a 20x4 text lcd

@bperrybap
Copy link
Contributor

There are many issues with this pull request - as is, I cannot accept it.
It would need quite a few changes and before it could be considered to be pulled in.
If you are up for doing all the necessary work to get this into the hd44780 library, then we can continue the conversion either here or somewhere else if you prefer.


An i/o class in the hd44780 library is much more than just a simple header file and a simple single .ino file.
Any new i/o class must be consistent in coding style in its source code modules to the rest of the library code as well as have its own full set of appropriately documented examples just like all the other i/o classes and be fully documented.

Before I could consider a pull request for a new i/o class, it will need to meet some additional requirements.

  • It must use Allman indention/bracketing/coding style to be consistent with all the rest of the other library code
  • It must have full example support like all the other ioClasses, (see hd44780_pinIO)
    For this ioClass it would be:
    -- hd44780Examples/LCDcharset
    -- hd44780Examples/LCDiSpeed
    -- hd44780Examples/LCDlibTest
    -- HelloWorld
    -- LCDCustomChars
    -- LineWrap
    -- ReadWrite
    -- Serial2LCD
    -- UpTime

And all those example files must be updated with appropriate comments documenting the i/o class.
i.e. how to use it, and anything specific to the i/o class to help the user understand how to use it in their own code.
It is more than just a simple cut/paste or copy & tweak of existing examples from another existing hd44780 i/o class.
All the examples for the i/o class will need to be modified for this i/o class.

There are readme files in the source code tree that will need to be updated.

  • the main README.md file that shows all the supported i/o class
  • the README.md file in the hd44780ioClass directory

The Documentation.ino sketch will need to be updated to reflect the new i/o class.

There is a wiki page for the i/o class that will need to be created
I will have to be involved with that since github doesn't support pull requests for the wiki and I don't allow anyone outside of the duinowitchery organization to update the wiki pages. i.e. if you initially write it, I'll add to the wiki.

This i/o class depends on another 3rd party library - currently no other i/o class has a 3rd party library dependency.
I'm not saying I would outright reject it because of this but it will need to be documented at a minimum in say in the HelloWorld and on the wiki page - maybe other places.
It will also need a small note to explain that even though the Adafruit library uses a BSD license the hd44780 library and this i/o class is GPL v3 so any code that uses this i/o class must be licensed as and subject to GPL v3
Also it will need to document things like how to install Adafruit_MCP23017_Arduino_Library which depends on Adafruit_BusIO
and how installing those two libraries is done can vary depending on the IDE version as older versions of the IDE do not support library install dependencies.

If the i/o class depends on a certain revision of the IDE (or newer), that creates some issues as well since currently the hd47480 library an all its i/o classes can run on any IDE version back to 1.0.1 - and is currently documented as such.
Assuming you decide to move forward with this, then any IDE version dependency for this i/o class would need to be documented.
Likely in the main README file, the Documentation.ino sketch, the wiki page, and all the examples.
Also, the IDE version would need to be checked for inside the ioClass header file and any examples included and an appropriate error message emitted if the IDE version is not appropriate.

Beyond that, there are some issues with the actual code.
The source code for the header file started with the hd44780_pinIO.h code so it is not original code.
The copyright and history sections of the source code will need to reflect this origin.

There is no need to do the keypad check since that is specific to the LCD keypad shield which does not apply to this h/w.
i.e. doing blPintest() and having any code to support the LCD keypad h/w issue work-around makes no sense so that code should be removed.

i/o classes are implemented as .h files to allow future templating.
Since they are .h files they are included with the users sketch code and are compiled with their code. As such they should not need to include any other header file.
Other header files needed should be included in the users sketch and included prior to the i/o class header file.
hd44780_pinIO did include <pins_arduino.h> but that was for the backlight dimming support which does not apply to this i/o class.
i.e. hd44780_mcp23X17.h should not be including any other header file.

If you don't want to implement backlight support you don't need to provide a iosetBackight() function.
The hd44780 class has a default virtual function to return the appropriate error if the function is not defined in the i/o class.

In terms of initialization, a high priority goal of the hd44780 library is that the only thing that should have to be changed when switching between i/o classes (LCD device h/w interfaces) is things outside any of the sketch code.
Things like the header files, the constructor, and possibly some other h/w declarations.
This is to provide a totally portable environment for the user code.
i.e. they write the sketch code once and can change to another LCD device on a different h/w interface by simply changing the constructor and possibly a few other things outside their sketch code.
So far that can be done with all the other existing i/o classes.
I'm not sure that it can be done for this i/o class given the way Adafruit did the initialization of their library using different begin() functions. (I would not have done it the way they did it, - But I've seen them use this method in other libraries)
Its not the end of the world, but not having the ability to just switch LCD devices & interfaces outside of sketch code creates an ugly inconsistency for this i/o class vs all the other i/o classes in the hd44780 library as it requires modifying actual sketch code to change between LCD devices.
I'd have to think about this for a bit to see if it is even possible to avoid this using some sort of template/wrapper that can set things up for the ioinit() function to do the mcp initialization there to keep it out of setup().
I'm not particularly a fan of Adafruit code. I've seen many cases where while it tends to provide some interesting functionality, it tends be bloated and have poor performance.
From what I can tell with this mcp code, it is going to be quite bloated in that it looks like it is always going to include all the SPI and I2C code in the image regardless of whether it is used.
Not a big deal for ESP parts but it can be for AVR parts.

Another big concern I have is the io class's dependency on an object outside the i/o class code that is declared & defined inside the user sketch code.
i.e. the Adafruit_MCP23X17 mcp object.

Not saying I would outright reject the i/o class because of this but it does create some concerns.
Also, of particular concern is that the i/o class cannot currently support more than a single LCD since the i/o class code is hard coded to use single mcp object.
Beyond being limited to a single LCD device my concern is that if the i/o class were to be updated to support multiple LCD devices/objects in the future the API (the constructor) may not be backward compatible with the current constructors.
I want to see consistency and do not want have a situation where there is a future backward compatibility issue between an early version of hd44780_MCP23X17 and later version.

The issue of single vs multiple LCD support can have a big impact on examples and documentation.
I would really prefer that the i/o class support multiple LCDs right at the start as all the other i/o classes support multiple LCD devices.

So that is kind of a high level view of what I'd be expecting from a new i/o class to be added to the library.
It is quite a bit of work to add a new i/o class, and then the testing of it across many h/w platforms to make sure it works.
i.e. AVR, ESP8266, ESP32, ChipKit, Teensy, ARM, etc....


Looking forward, here is some news on the hd44780 library.
I'm in the process of changing the directory structure of the library.
It moves around where things live an the library tree, which is about trying to make things clearer and easier for the user.
Things like more clearly identifying the examples is major goal of this.
It has no affect on the actual i/o class code in the header file or the examples - just changes where they live.
This will bump the library major version up to 2.x

I'm also in the process of adding some new i/o classes to replace the current i/o class that use i2c interfaces.
Thew new i/o classes will use templates to allow the user to specify which i2c bus and which i2c "Wire" object to use.
This allows the i/o class to work with any "Wire" compatible library (i.e. Wire, SoftWire, etc...) as well as any "Wire" object (i.e. Wire, myWire, Wire1, Wire2, etc...)
This means that the "Wire" object class and its name will not longer matter since it will be passed in to the template as parameters.
I would have preferred to alter the existing i/o classes but there is no way to do this and maintain backward compatibility given some external technical limitations in the IDE and gcc toolset version(s).

@jmiguelf
Copy link
Author

I just don’t have the time needed :/

@bperrybap
Copy link
Contributor

Yeah, it is a bunch of work, to fully integrate into the library.
I'll leave this open for now for a while in case you change your mind.

I'm curious what the performance for that i/o class is.
i.e. the bytexfer time.
You can measure it if you setup the LCDiSpeed sketch under the hd44780examples directory in the i/o class examples.

@jmiguelf
Copy link
Author

ByteXfer:1315uS

20x4FPS: 9.05
Ftime: 110.44ms

16x2iFPS: 22.64
iFtime: 44.18ms

how is this better or worse than the direct io?
(i dont have any arduino to compare )

@bperrybap
Copy link
Contributor

Here are some numbers all run on ESP8266 which is what I believe you used as well.

ESP8266 D1 mini I2C using hd44780_I2Cexp
 PCF8574 @ 100khz ByteXfer: 487us
 PCF8574 @ 400khz ByteXfer: 130us
MCP23008 @ 100khz ByteXfer: 578us
MCP23008 @ 400khz ByteXfer: 154us

ESP8266 D1 mini i2C using hd44780_I2Clcd
AIP31068 @ 100khz ByteXfer: 313us
AIP31068 @ 400khz ByteXfer: 93us

ESP8266 D1 R1 (UNO form factor with LCD shield) 4 bit mode with Direct pin control using hd44780_pinIO 
ByteXfer: 52us

You can see direct pin control in 4 bit mode is much faster at 52us
Some optimized libraries that bypass the digital i/o APIs and use indirect port i/o (not direct port io) will get byte transfer times down around 17-22us.

The i2c MCP23XXX parts will always be slower than the PCF8574 given it requires a byte across the bus to set up the port register for every data transfer - which in 4 bit mode is every nibble.
But even over i2c at a much slower clock rate than the SPI clock rate with much more overhead bytes than SPI,
the byte transfer times to the LCD are are still much faster than what you will get when using the Adafruit library.
(SPI was configured to transfer at 1 Mhz).

Not surprised that the numbers are so slow given the mode of operation and using the Adafruit library.
It could be much faster if the code didn't use the Adafruit library to mimick the Arduino digital pin i/o API. i.e. pinMode(), digitalWrite(), digitalRead()

I pretty much lost interest in the MCP parts when the PCF8574 based backpacks started coming out for less than $1 USD shipped to your door.
They are readily available, super cheap, and easy to use with the hd44780 library given it will auto locate the i2c address and automatically determine the pin wiring between the PCF8574 i/o port pins and the LCD pins so the user doesn't have to configure anything.

In this case, SPI uses more pins, the pins have to be configured by the user, and it ends up being significantly slower than a super cheap ready made i2c solution.
And on the AVR, the SPI hardware ties up hardware SS pin. So if you use a different pin than the hardware SS pin, that is another pin since it can't be used for a generic purpose as it can affect the SPI hardware transfers.

@jmiguelf
Copy link
Author

I’m using it just because it’s the simplest solution, I’m already using spi, so only one pin more (only one pin free on my setup).

@jmiguelf
Copy link
Author

One question, why you’re not initialize hardware only in lcd.init?

@bperrybap
Copy link
Contributor

Not quite sure of your questions about lcd.init()
If you are asking why the cols and rows get initialized that is because it has to do that for some of compatibility provided - below for details.
If you are asking why use begin() vs just init() see below.

From an API perspective, there is a long history of the the APIs used by Arduino libraries to control a hd44780 LCD.
There is LCD API 1.0 and then there is "LiquidCrystal"
These go back more than 10 years.
https://playground.arduino.cc/Code/LCDAPI/
https://www.arduino.cc/reference/en/libraries/liquidcrystal/

LCD API 1.0 was created as an attempt to standardize an API since at the time things were all over the place with not common API. hd44780 LCD libraries used very different APIs, and initialization.
There was a LiquidCrystal library but it was in the very early stages of development.
This is back like 13-15 years ago.

The APIs are similar but slightly different. The "LiquidCrystal" API is what has been implemented in the LiquidCrystal library.
The API in this library and its structure has changed slightly through the years and most of those changes resulted after Adafruit took control of the library for a while.
There are also some incompatibility issues between the two like the two setCursor() API calls have their arguments in reverse order.
Also, LCD API 1.0 used a design that set up the geometry in the constructor and a call to init() would initilize the LCD.
LiquidCrystal used a design that only set up the control h/w configuration in the constructor then used begin() to pass in the LCD geometry parameters.

The LiquidCrystal library calls its low level h/w initalization in its init() function.
This function is called from the library constructors and also sets up a default display geometry of 16x1
This was to allow the user to just start using the LCD without ever having to call any sort of initialization function if they happen to be using a 16x1 display and then they could call begin() if they had a different geometry of font size.

This is very problematic.

  • it creates an API inconsistency depending on the geometry.
    For consistency users should simply always call the begin() function.
  • There is no need to support this 16x1 default mode since many users won't be using that type of display and will need to initialize the
  • doing h/w initialization won't work when other h/w interfaces like i2c are used to access and control the LCD.
    Due to the way and when in time C++ calls the constructors and when the Arduino environment is setup, you can't do anything inside a constructor that needs to use certain resources or needs to use interrupts. It can't be done.

To work for all h/w environments, the library API needs an initialization function that can be called from the user sketch code, typically the setup() function. This is late enough that any Arduino resources can be used as well as interrupts.

So the issue of init() vs begin() comes down to which API.
LCD API 1.0 used init() with NO parameters to provide a consistency across all h/w interfaces.
LiquidCrystal provides a begin(row, cols, [font]) API call to provide a consistency across all h/w interfaces.
LiquidCrystal also provides an init() function:

void init(uint8_t fourbitmode, uint8_t rs, uint8_t rw, uint8_t enable,
	    uint8_t d0, uint8_t d1, uint8_t d2, uint8_t d3,
	    uint8_t d4, uint8_t d5, uint8_t d6, uint8_t d7);

but it really is an internal function.
It is inconsistent with its own constructors, and never should have been made public.
It is very confusing since for 4 bit mode you put the arduino pins for d4, d5, d6, d7 in the function as parameters for d0, d1, d2, d3
This was STUPID! But like I said the LiquidCrystal API function init() is really an internal function and should never be used by sketch code.

If the intent was to provide an init() function to comply with LCD API 1.0 then it was a total failure since what they did is not how LCD API1.0 defined init() nor can it ever be portable across platforms.

The hd44780 library accommodates both APIs as much is technically possible.
It uses the LiquidCrystal setCursor() vs the LCD API 1.0 setcursor()
It provides the LiquidCrystal begin() function vs the LCD API 1.0 init() function since LiquidCrystal code is much more prevalent, it provides a portable function that works across all h/w interfaces.
And by using the begin() functions, the constructors for the hd44780_pinIO class can be parameter compatible with LiquidCrystal for easy porting.
It dos not provide a LiquidCrystal API init() function as well, that API function is just plane silly and never should have been made public.
Well technically hd44780 does have a init() API function. It is not intended for normal use and it is not documented in the API documentation. This along with a LiquidCrytal_I2C compatible constructor in hd44780_I2Cexp was added to provide additional compatibility with the LiquidCrystal_I2C library API and existing LIquidCrystal_I2C sketches.
This allows a user to only change the class name on the lcd object constructor and their existing code will "just work".
It is a crutch to get peoples existing LiquidCrystal_I2C code up and running even if they were using the incorrect constructor parameters and initialization API function when switching to hd44780_I2Cexp
i.e. they can use LiquidCrystal_I2C constructor parameters and call init() - both of which is incorrect for hd44780_I2Cexp but the code will still build and work.

@jmiguelf
Copy link
Author

My mistake, I wanted to say lcd.begin…
I personally don’t like to have any hardware init code in the class constructor, I just think it’s a little stupid, for example on my case, I only want to use half of the ic.. so I just need to pass the mcp to the class, but only works after initialization. The way this lib is made , is perfect for 99% of the users, but not for me.
I don’t like the lib I’m using for the mcp, but it’s the only one ready to use.
About the lcd timings and hardware interface, why not use the busy flag instead the delay()?

@bperrybap
Copy link
Contributor

bperrybap commented Nov 23, 2022

The way this lib is made , is perfect for 99% of the users, but not for me.

Not sure I follow.
When you say "this lib" I assume you are meaning the Adafruit MCP lib or are you meaning the hd44780 library?
If the hd44780 library what is it that doesn't work for you?

About the lcd timings and hardware interface, why not use the busy flag instead the delay()?

Because it is slower.
Just think about it, for all LCD instructions except clear() or home() it will take longer just to poll the BUSY bit one time than the instruction takes to execute.

On the AVR even with direct pin control using the digital i/o API (digitalRead(), digitalWrite(), pinMode() )
will be MUCH slower than the instruction time.
it takes 3.5-6us for each call. To read busy, it takes 8 calls to flip all 8 pins from output to input,
Then read the pin on DB7, and then 8 calls to flip all 8 pins from input mode back to output mode.
That is 8 x 4 + 4 + 8 x 4 or around 70us just to check BUSY one time. The instruction time is only 37us.
And that assumes zero time for the actual other overhead and doens't account for the control lines which also have to be changed.

If using 4 bit mode, it is still the same problem because while you only have 4 data pins, you now have two nibbles
along with R/W, RS, and E to deal with.

Now for clear() and home() there could be a a benefit, assuming the LCD processes it in less than 2ms.

Other interfaces can't send the data as fast as the instruction time.
For example with i2c using a PCF8574 the time to transfer the bytes over to set up the output port takes longer than the instruction time. It would make no sense to poll BUYS over i2c as it would be WAY slower than doing it with direct pin access which is already slower than the instruction time.

The hd44780 library is smart about its delays.
It does not do blind delays. It allows the LCD instruction execution to run in parallel with Arduino processor code.
This can hide some if not all the library and h/w interface overhead depending on what is used and what is being done.
For example, if the user's code were to call clear() the library comes back immediately with no waiting.
If the Arduino processor spends 2ms doing something else and before the sketch code prints a character, then there will be no delay before sending that character. Even though BUSY is not used, the library only waits when necessary as it waits for the previous instruction to complete if enough time has not already passed for it to complete.
In some i/o classes it also takes into consideration how long it takes for the next instruction to be handed to the LCD through the h/w interface connection.

Other LCD libraries do a blind delay for the full instruction time immediately after sending the instruction.

I would not use the Adafruit library when using a MCP23XXX to control an LCD.
I would talk to the MCP chip directly.
It isn't that difficult.
For speed and simplicity I would run run the LCD in 8 bit mode and use the lower port for the control lines and the upper port for the data lines.
You can then optimize the transfers by sending control, then data.
And to control E just send the byte for lower port with the LCD control lines.

If using the hd44780 library, the i/o class source code would likely be smaller (fewer lines of code) than what you have now, for sure if you tossed out support for reads.
The hd44780_I2Cexp i/o class supports the MCP23008
I never did an i/o class for the dual port MCP23XXX chips. I didn't see people using it.
Although I have seen an Adafruit RGB keypad shield that uses it.

As I recall those dual port MCP chips have a h/w issue in their design where you can't tell what register banking mode the chip is in which can can and will cause initialization issues, if you use one of the banking modes to optimize performance.

IMO, it is a major screwup.

---- bill

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.

2 participants