I2C Display Support - Githubissues

Traumflug commented 8 years ago

It escaped me for a while, somebody pushed code for I2C display support to the repo recently. Thank you for this!

Now, the unfortunate thing is, judging by the commit messages it apparently doesn't work, yet, still it went straight to branch experimental. So I did the following:

Made a copy of experimental and named it i2cdisplay.
Pushed this copy.
Restored the former branch experimental.

With this done, it's a proper topic branch now. For those who fetched/pulled experimental recently, please do the following:

If you made local commits on top of experimental, make patches from them with

git checkout experimental
git format-patch --keep-subject origin/experimental

In any case, remove the local branch experimental then:

git branch -D experimental

Then restore from the remote branch:

git fetch
git checkout -b origin/experimental
git checkout -b origin/i2cdisplay

In case there were local branches, apply them on top of i2cdisplay with git am.

After having this done, branch i2cdisplay is exactly what was experimental before.

This new branch has a number of, well, messy commits, but also quite a number of good ones. I'll try to sort this before too long. The general implementation strategy looks very good!

... now scratching my head where I could find a device supporting I2C to allow testing ... :-)

RaD commented 8 years ago

Do you mean my module https://github.com/Traumflug/Teacup_Firmware/blob/ssd1306/i2c_bus.c ?

Traumflug commented 8 years ago

This module, and some 50 additional commits. It's solved now.

Uhm. I still have no I2C device. Is it correct your display would work on a generic, ATmega based controller as long as it has an I2C connector? Uhm, could you afford to send me one?

RaD commented 8 years ago

No problem, which kind of SSD display do you want? 5V or 3.3V? I still have no time to finish with I2C queue, other stuff works pretty well, I have sent a photo in appropriate issue: bkx07j3d78i Send me your postal address via rpopov/skype.

Traumflug commented 8 years ago

Wohoow, a pixel based display! Interesting what you managed to squeeze into this small '328.

RaD commented 8 years ago

It is smart display, it has own RAM. So I can send info by small chunks. Check docs on SSD1306 128x32 I2C on Adafruit site.

RaD commented 8 years ago

As @Traumflug said, the OLED screen is in his home. I hope the queue problem would be solved soon.

Wurstnase commented 8 years ago

Great work!

RaD commented 8 years ago

Still have no time to make right merge my I2C code with teacup queues.

Traumflug commented 8 years ago

First, thank you for sending me a display. I got it working now to some extent. I2C appears to be a bit more complicated than serial or SPI, one has to stick to a protocol! :-)

Looking at the code I wonder wether this is a proven library fetched from somewhere or wether you wrote this yourself. If it's a library it likely comes with some sample code.

One issue I found is that the TWI interrupt never stops, causing buffers to overflow. Putting a stop there and suddenly a lot more works. Still I'd like to know how to properly stop I2C/TWI, so far I simply turn it off.

Wurstnase commented 8 years ago

Finally I used u8glib at work. This works but needs more space.

Traumflug commented 8 years ago

@RaD, I just pushed branch i2c-rebuild, so you can see what happens. Basically I picked the first commit from your branch and picked together a test case from your other commits. Then refined the stuff until it worked, all on the first commit. After that I added a number of simplifications. On how to enable the testcase see the top comment in i2c_test.c. Here it successfully writes a message to the screen.

Problem at this point is that only entire blocks can be sent. This makes e.g. clearing the display a wasteful operation, one had to create a buffer with 512 zeros. It also prohibits sending from program memory. As far as I can see this limitation is the problem you got stuck on.

Next planned step is to implement a flexible buffer, like serial uses it already and like you tried on. Not trivial, because I2C wants an explicite transmission start and transmission end. Simply blasting out bytes like with serial or SPI doesn't work. So far I think the best solution is to split the send handler into ...start, ...write and ...end. All this compatible with SPI.

When this works, too, all the other goodies you wrote can join. The implementation for Configtool looks just fine!

BTW., this display is kind of funny. Automatic scrolling supported, but not screen clearing. And it keeps its contents even without supply voltage. Turn the thing off and, hours later, on again and the last screen content is still there :-)

RaD commented 8 years ago

Please, take a look on https://github.com/halfakop/Teacup_Firmware/commit/6343a22a4dfebaeec782028eb65a989e64fa4317

I have done some unfinished work there.

There is no problem with big block because of you can send many small block. For instance, to clear display it would be enough to send 64x of 8 bytes zero blocks, one by one.

My problem is how to make on C lang flexible buffer that will take fast messages from other firmware and slowly (i2c is not fast) send to display. Now I am busy little bit with other tasks but I hope I will return soon.

Traumflug commented 8 years ago

Please, take a look on halfakop@6343a22

Thanks. If I can't find a better solution, which is quite possible, we'll fall back to this. BTW., your block there is on the stack, which will go away before all bytes are sent. It should be declared 'static'.

One thing I couldn't find out so far is, how does the display distinguish between pixel data and commands? Unless I'm mistaken there are no data length fields and also no end-of-data markers. So far my assumption is that the distinction happens on the I2C level: each transmission start starts with a command and a transmission end is also a data end.

RaD commented 8 years ago

First byte of packet: Co|Dc|0|0|0|0|0|0.

After sending I2C address of a device you may send command or data. If Co=0, then data bytes would be send. If Dc=0, next byte would be command. If Dc=1, next byte would be send to display video memory, pointer there will be incremented.

Pay attention on data marker: https://github.com/halfakop/Teacup_Firmware/blob/ssd1306/display_ssd1306_i2c.c#L70

RaD commented 8 years ago

At shortly: 0x00 command there 0x40 data bytes

Traumflug commented 8 years ago

Yes, but how many data bytes? Once bytes are sent to display video, there's no apparent way to stop this for returning to the command level.

RaD commented 8 years ago

Until I2С packet ends.

Traumflug commented 8 years ago

Glad to see you came to the same conclusion :-)

This ringbuffer is now implemented and pushed. No reliable transmission end detection so far, but test code clears the screen just nicely with a simple loop.

RaD commented 8 years ago

I think that display interface should give users the following function: https://github.com/halfakop/Teacup_Firmware/blob/ssd1306/display_ssd1306_i2c.h#L39-L47

I mean that firmware doesn't do print('t=230') for hotend but just update DISPLAY_T structure and "display driver" itself prints all data in right display positions.

Traumflug commented 8 years ago

I think that display interface should give users the following function: https://github.com/halfakop/Teacup_Firmware/blob/ssd1306/display_ssd1306_i2c.h#L39-L47

Yes, absolutely. This is a higher level than what I'm currently on. Right now I work on the I2C part. Serving the display should be independent from this, because the same or similar displays can be connected via each of the protocols (I2C, SPI, 4-bit, 8-bit).

Next step will be, I think, a display connection broker. Like

#if defined DISPLAYBUS_I2C
  #define displaybus_write(x) i2c_write(x)
#elif defined DISPLAYBUS_SPI
  #define displaybus_write(x) spi_rw(x)
#elif ...

Having this, dependencies of display code on the connection protocol is removed. _display_ssd1306i2c.c becomes _displayssd1306.c and uses _displaybuswrite() instead of _i2cwrite(). All without any binary size increase or additional runtime calculations.

No such golden solution so far for re-using sersendf() for writing to the display. Right now, sersendf() writes results straight to the serial line. It's output channel has to be switched somehow. Not sure wether this should be done by a global switching function, which has to be called before writing to a specific channel, or by carrying around the channel as a parameter, similar to fprintf(3).

Traumflug commented 8 years ago

Good. I2C part done, bus working reliably in all test conditions I could set up. Ringbuffer working fine as well. @RaD, I'd be glad if you could run this code, too, it'd be not the first time code working perfectly here fails in some detail elsewhere.

welcome to teacup

BTW., this display is apparently light (opposite of dark) enough to make everything else around it dark on a photograph. I had to photshop lighting in the above picture substantially to get the surrounding visible again.

Controller is my trusty Gen7 1.1, these black/white ringed wires are part of my steampunk (and working!) SD card adaptor.

Traumflug commented 8 years ago

Just a note: in Adafruit's library they put a 0x00 in front of each single command, even when sending several of them together. Example.

RaD commented 8 years ago

About the note. This is allowable behaviour but unneeded.

Wurstnase commented 8 years ago

I've pushed my u8g-stuff. Maybe you can use some lines of it. There are 2 buttons implemented. Not very pretty, but better than nothing. https://github.com/Traumflug/Teacup_Firmware/tree/u8g_with_buttons

Traumflug commented 8 years ago

This is allowable behaviour but unneeded.

So you think it's pointless to support any display but yours. I better stay calm now. And thank you for testing the reviewed work.

Traumflug commented 8 years ago

Let me try to explain why the code from halfakop didn't work. The bug, at least one, is here This shouldn't be I2C_MODE_SARP, but I2C_MODE_SAWP.

Another bug is plain exhaustion of RAM, at least on the '328. While my test code isn't exactly the suggested one, it's close to it. And this https://github.com/Traumflug/Teacup_Firmware/commit/f32c23d422db8d39ac21f9645cb55a5cd5aba786 shows that all these buffers simply don't fit into these 2 kB RAM the '328 features. Even if it would just fit, there was zero space for stacks (memory allocated inside functions). The same commit also shows the solution: write directly from program memory.

Then I saw the implementation to queue up entire transmissions. Yes, excellent idea ... if there was enough memory to allow this. If one buffer doesn't fit into RAM, 8 of them fit much less.

That said, it's a bit questionable wether such a buffer queue makes sense. Memory has to be copied around, the queue has to be maintained, which costs CPU cycles, binary size, RAM. Sending a byte over I2C costs 9 I2C clocks, or about 360 CPU clocks at maximum I2C speed. This isn't much, one might end up spending more time maintaining the queue than saving elsewhere. That's why I don't plan to pick this part, I hope you understand.

Traumflug commented 8 years ago

I've pushed my u8g-stuff.

How much RAM does this cost? How much Flash does this cost? What can it do what the current code can't? Does it work on small AVRs?

Wurstnase commented 8 years ago

This code runs on an Arduino Nano. But I mainly pushed it for the buttons and the idea how to shift visual pages. When I remember correctly I'm at ~20k. Maybe a bit more. But I deactivate lookahead and some other things could be optimized out, while I don't use any stepper. (take a look at commit HEAD~2)

With acc_ramping and lookahead:

    SIZES             ATmega...  '168    '328(P)    '644(P)    '1280
    FLASH  : 26896 bytes         188%        88%        43%      21%
    RAM    :  2023 bytes         198%        99%        50%      25%
    EEPROM :     0 bytes           0%         0%         0%       0%

Traumflug commented 8 years ago

I write and write and write ... just pushed a bunch of improvements / cleanups to Configtool on branch i2c-rebuild. 28 commits now and still only 50% of @RaD's work picked.

phord commented 8 years ago

I just dropped a bunch more for you to consider. :boom:

Traumflug commented 8 years ago

Just pushed current state. The last few commits aren't the last word, but now the distance to experimental is a lot shorter.

Traumflug commented 8 years ago

Also removed branch ssd1306. These two commits are on experimental now (and a few other branches).

Traumflug commented 8 years ago

Another chunk on experimental.

Infrastruture is mostly done, prepared for display bus and display type selection.
sersendf_P() replaced by sendf_P(), which can write to both, the serial line and the display. A convenience macro sersendf_P() still exists, so no need to change code using it.
I2C error handling is now successful. I2C turned out to be pretty unreliable when running at the "wrong" bitrate, at least on my test board. At 100'000 bits/s I experience about one transmission loss per second; no such odds when running at 200'000 bits/s. Looks like there's a need to make this user selectable.

Traumflug commented 8 years ago

D'oh. Display hardware decided to say bye bye shortly before I was done. So I had to put the remaining plan into a large comment in _displayssd1306.c, see there. Latest achievements:

I2C is now reliable. I misunderstood one of the bitfields, code was actually wrong.
Display should now show a greeting message and X/Y/Z coordinates updating once a second.

Remaining parts shouldn't be too difficult to do. The queueing mechanism needs another tweak (see this comment), everything else is a matter of arranging things in a neat fashion.

All code on experimental, branch i2c-rebuild removed. I plan to remove the other related branches (i2cdisplay-review, i2cdisplay), too, after searching them for nuggets I missed so far.

I'm out, somebody please take over!

Traumflug commented 8 years ago

Big spring cleaning today, branch i2c-display removed as well, after verifying that all its nuggets are on master.

Wurstnase commented 8 years ago

Just tried this with the Adafruit Display. But can't get this to work. I changed the address to 0x3C and copied the u8g working init_sequence but unfortunately I don't see anything.

Any hints, how I could debug this?

Traumflug commented 8 years ago

I2C is a tricky thing. Other than SPI it's an outright protocol.

Regarding the address, it's not an 8-bit address, but a 7-bit one. The last bit is a read/write flag. To make this a byte, one has to shift the address one to the left. 0x3C << 1 = 0x78. See http://www.i2c-bus.org/addressing/

If this still doesn't work it's time for some printf() style debugging to see wether bytes are sent at all. Wether the interrupt happens, with which status it happens, such stuff. During writing that code I used a few serial_writechar() with different characters to see what happens. I2C bus speed can be reduced to not overwhelm the serial line with debugging characters.

Wurstnase commented 8 years ago

Oh dear! I changed the address first and thought this was correct. Back to 0x78 and taking the new init sequence and everything works as expected. Thank @Traumflug

Traumflug commented 8 years ago

This was quick :-) Excellent!

Wurstnase commented 8 years ago

Or not :/ Friday anything looks perfect. Monday nothing works... How should the debug looks like? This is what I get: .1.3.4.4.4.4.1.3.4.4.4.4.4.1.3.4.4.4.1.3.4.4.4.4.

Wurstnase commented 8 years ago

Some progress: I flash the Nano with the adafruit example. (GFX etc.) Display shows the flash screen etc. Now I flash Teacup and it works. I have connected the reset pin and I guess it's something with this?!

Wurstnase commented 8 years ago

Sorry for the noise. It is the reset pin.

Flashed adafruit example. It works. Flashed Teacup. It works. Unpower Nano Power Nano It doesn't work.

Wurstnase commented 8 years ago

Really, really close to find the issue. The problem could be the displaybus_write(0x40, 0); for the init sequence in display_tick(). Currently I'm trying some extra code. I have a second init_sequence but when I try to go through this I only get zeros. Any idea what I'm doing wrong?

static const uint8_t PROGMEM init_sequence_2[] = {
  0x00,
  0x21, /* column addr (0 =  reset) */
  0x00,
  0x7F, /* column end address (127 = reset) */
  0x22, /* page addr (0 0 reset) */
  0x00,
  0x03  /* page end addr*/
};

and later:

for (i = 0; i < sizeof(init_sequence_2); i++) {
  sendf_P(serial_writechar, PSTR("%su\n"), (uint8_t)init_sequence_2[i]);
}

Any init_sequence_2[i] is zero.

Traumflug commented 8 years ago

You read out RAM, not Flash.

  sendf_P(serial_writechar, PSTR("%su\n"), (uint8_t)init_sequence_2[i]);

==>

  sendf_P(serial_writechar, PSTR("%su\n"), (uint8_t)pgm_read_byte(&init_sequence_2[i]));

phord commented 8 years ago

Good catch, @Traumflug !

Also, you can drop the cast from pgm_read_byte since it already returns uint8_t.

sendf_P(serial_writechar, PSTR("%su\n"), pgm_read_byte(&init_sequence_2[i]));

Wurstnase commented 8 years ago

Thanks, but this code works? https://github.com/Traumflug/Teacup_Firmware/blob/master/display_ssd1306.c#L111 displaybus_write(init_sequence[i], (i == sizeof(init_sequence) - 1));

Wurstnase commented 8 years ago

YES! pgm_read_byte solves everything!

Traumflug commented 8 years ago

Thanks, but this code works?

Looks like you catched a bug over there. My fault.

Wurstnase commented 8 years ago

https://github.com/Traumflug/Teacup_Firmware/tree/fix_ssd1306

waits for a rebase to experimental :+1: I will push tomorrow the init code for the Adafruit display. It's not much anymore what that display needs.

Wurstnase commented 8 years ago

Added support for Adafruit SSD1306 and added new font 8x8.

Traumflug / Teacup_Firmware

I2C Display Support #190