i2c, ws2812, timer3 conflict?

[mannkind]

This is not a bug or issue, it's just a question.

I'm trying to use I2C to control the leds on the slave half of a hardwired lets split keyboard with ws2812 leds. It's working, for the most part, except for the led animations. The animations only seem to work on whichever half is the "master" keyboard.

Is there a conflict between the I2C/ISR and the Timer3/ISR?

I swear I've searched for the issue, but I've come up empty handed. I have a feeling this might be why all the examples I've found use one-wire serial communication between the two halves and connect the RGB_DI_PINs via the unused TRRS line.

[jackhumbert]

Which keyboard is this for? You'll need timers on both halves - right now I don't think the animation stuff is sent over I2C, but that may be able to be changed.

[mannkind]

Sorry if I wasn't clear, it's for a "Let's Split" keyboard that I've hand-wired – it uses two atmega32u4 microcontrollers.

Right. It doesn't seem like any rgbled_light stuff is currently communicated back to the slave keyboard via I2C. The code as-is only controls the master keyboards LEDs.

FWIW I seem to be able to toggle the slave LEDs on/off & change HSV via I2C using a hacky little mod (which is probably wrong because I'm not familiar with the codebase), but changing to a mode that enables timer3 doesn't seem to work on the slave.

[ahtn]

The i2c code doesn't rely on anything that should conflict with the timer3 interrupt that I am aware of.

Have you made sure your code is actually running on the slave? When the slave starts, it runs the code in this loop.

The i2c slave code behaves similar to i2c eeprom devices. It's essential just an array of memory and a pointer into this array.

uint8_t slave_buffer_pos;
uint8_t i2c_slave_buffer[SLAVE_BUFFER_SIZE];

The first write of an i2c transaction sets the position of slave_buffer_pos. Subsequent read/write operations access i2c_slave_buffer[slave_buffer_pos] and auto increment slave_buffer_pos.
For example, the slave stores the keyboards matrix data at 0x00 in the array
and is accessed like this.

[mannkind]

Thank you both for your responses. I do appreciate it.

The i2c code doesn't rely on anything that should conflict with the timer3 interrupt that I am aware of.

Darn... that was the conclusion I was coming to from all my Google searches, but I wasn't sure. I fully admit to being a bit out of my depth here.

I'm positive my code is running on the slave, because without my hacky patch nothing at all happens on the slave keyboard w.r.t. the LEDs.

The first write of an i2c transaction sets the position of slave_buffer_pos. Subsequent read/write operations access i2c_slave_buffer[slave_buffer_pos] and auto increment slave_buffer_pos.
For example, the slave stores the keyboards matrix data at 0x00 in the array
and is accessed like this.

Right. I found/modified that bit of the code. I couldn't figure out how the master incrementing the slave_buffer_pos would be useful in communicating that I wanted to take an action on the slave keyboard; it didn't seem like it was even used as nothing seemed to change when I commented it out.

[ahtn]

When you say it doesn't work, what does that mean? Do keypresses work on the slave side? Do animations start on the slave side and then everything else stops working? After a look at the code, I can see at least one way the i2c and rgb isr will conflict, but I can't say if this is the only problem.

Possible cause of problems

After having a look at rgblight.c, I'd say the problem is that the i2c operations are timing out because timer3 interrupts takes a long time to execute (I guess maybe 10's of µs to 100's of µs???). Since ISRs disable other interrupts until they finish executing, the timer3 ISR will probably be blocking the i2c intterupt. That max wait time for i2c communications is also chosen to be minimal which won't help either.

Quick and dirty fix that might not work

For a quick and dirty fix that might work, you could try enable interrupts as soon as the timer3 ISR starts, that is add sei(); here. And maybe increase the max wait delay for i2c.

Correct fix

IMO, the correct way to get these two things working together would be to rewrite rgb code so that all the animation functions take a time as an argument rgb_animation_func(time) and call this function each time the matrix is scanned. The host would then run a simple interrupt that just increments a counter that controls the animation (or just use QMK/TMK's builtin read_timer()).

Then, instead of using an interrupt to keep track of the time on the slave, send the value of the time used by the host to the slave. This has the advantage that the host and the slave will be syncronized. The slave can then control the LED animations in the same way as the master.

These changes wouldn't be too hard to implement since the current implementation already uses read_timer internally, to control animations. To be honest, I'm not even sure why the current implementation even uses timer3.

[mannkind]

Again, thank you for your time and info!

When you say it doesn't work, what does that mean? Do keypresses work on the slave side? Do animations start on the slave side and then everything else stops working?

The animations on the slave do no start (they start on the master); the key presses are still communicated as expected.

Some output + comments from hid_listen.mac

// Initial state; mode 1
rgblight mode: 1
TIMER3 disabled.
rgblight set hsv [EEPROM]: 0,51,238

// Using RGB_ keys to change HSV; these are reflected on the slave as expected
Sending data to slave: 7
rgblight set hsv [EEPROM]: 0,51,255
Sending data to slave: 5
rgblight set hsv [EEPROM]: 0,68,255

// Changing the mode; the master LEDs start "breathing", the slave remains solid
Sending data to slave: 2
rgblight mode: 2
TIMER3 enabled.
rgblight set hsv [EEPROM]: 0,68,255

// Toggling the LEDs - both master and slave LEDs turn off
Sending data to slave: 1
rgblight toggle: rgblight_config.enable = 0
TIMER3 disabled.

// Toggling the LEDs again - only the master turns on (in "breathing" mode); slave LEDs remain off.
Sending data to slave: 1
rgblight toggle: rgblight_config.enable = 1
rgblight mode: 2
TIMER3 enabled.
rgblight set hsv [EEPROM]: 0,68,255

// The slave's LEDs won't turn on again until the master returns to 
// mode 1 and timer3 is disabled. Or if I switch the usb cable from the master to the 
// slave, then the former 'slave' LEDs will be in breathing mode. 

Quick and dirty fix that might not work

Thanks for this tip. I'll try it out tomorrow morning; maybe experiment with the I2C timeout as well and report back.

[ahtn]

If key presses are working, then the quick fix I suggested probably won't do anything.

[ahtn]

Okay, I think I found the problem. timer_init() is not being called on the slave since the slaves main loop runs before its intialised on the slave. Try adding timer_init() to keyboard_slave_setup(). (since rgblight code relies on timer_read())

[mannkind]

Wahoo! Adding timer_init() (in addition to the previous addition of rgblight_init()) to keyboard_slave_setup() made it so that the LED animations start as expected on the slave keyboard!

The addition also seems to cause the keyboard to repeatedly send keys from the first row of the slave keyboard (mostly 'y') to the computer, and to be slow to respond to key presses on the slave. Perhaps I need to try the "quick and dirty" fix you mentioned earlier…

[ahtn]

If you're interested, I cleaned up the i2c api so that all the internals of i2c are now hidden. I'll probably get around to making a pull request for QMK some time.

I added some test code that controls the slave promicro's RX LED:

• master code
• slave code

[mannkind]

Neat! I'm certainly interested.

I was able to integrate your i2c api and utilize your I2C_WRITE_TEST_CODE examples to send/recv key-presses to/on the slave keyboard. It's a lot easier to send actual keypresses with the api you created. :)

There still seems to be the issue mentioned above when timer3 is enabled on the slave. It stops when I remove the timer_init() code suggested earlier or switch the LEDs to a mode that disables timer3. It could certainly be a hardware issue, but I don't have the additional keyboard/hardware to test it with.

Also, I realize that this may be an unsupported configuration (controlling the LEDs via I2C); I understand if this needs to be closed as WONTFIX :)

[ahtn]

The issue you mention sounds like it is caused by the long running ISR for timer 3 that I mentioned before. When timer_init() is not called, the timer3 ISR looks like it will exit quickly, hence why it works without timer_init().

Try these changes to the rgb code, that make the animation code polling based. You'll need to add

rgblight_animation_poll(timer_read());

to the master's and slave's main_loops/matrix_scan functions. However, you would ideally pass the value of timer_read() from the master to the slave over i2c in order to keep the animations synchronized.

Edit: I don't have the hardware to test this, so some modifications may be necessary.

[mannkind]

FWIW, I tried both rgblight_animation_poll(timer_read()); and rgblight_animation_poll(value_passed_via_i2c); and the animations seem to work the same as the pre-change code as far as I can tell. The animations do diverge within a minute or two, even when using the value passed via I2C.

I don't want to conflate the issues if they're unrelated, but I'm still having the erroneously reported keypresses issue when animations are occurring on the slave - it doesn't matter which half is the slave and the frequency of the issue seems to be related to the speed of the animation.

For fun, here's an output from matrix_print() when the issue occurs; no keys are being physically being pressed.

r/c 0123456789ABCDEF
00: 0000000000000000
01: 0000000000000000
02: 0000000000000000
03: 0000000000000000
04: 1100110000000000
05: 1100110000000000
06: 0000000100000000
07: 0000000000000000

r/c 0123456789ABCDEF
00: 0000000000000000
01: 0000000000000000
02: 0000000000000000
03: 0000000000000000
04: 1100110000000000
05: 1100110000000000
06: 0000110000000000
07: 0000000000000000

r/c 0123456789ABCDEF
00: 0000000000000000
01: 0000000000000000
02: 0000000000000000
03: 0000000000000000
04: 1100110000000000
05: 1100110000000000
06: 0011001100000000
07: 0000000000000000

All that said, I don't know how much more of anyone's time I should waste if I'm the one of the only people trying to use this 'feature' :) There seems to be a decent work-around – I've tested the Serial setup and used the (now)spare conductor in the TRRS cable to control the LEDs. That seems to be working, LED animations included, without any major issues … though technically slower, I guess.

[ahtn]

I can't really make any valid judgement without seeing the exact code you used. It sounds like you got the setup you wanted anyway with the serial setup.

though technically slower, I guess.

Using i2c would only shave off >1ms of latency in your setup, so it's not like you would notice. You'd only really need it if you wanted to try for more than 2-way split or something else fancy.

[mannkind]

I can't really make any valid judgement without seeing the exact code you used.

I understand. I do have a minimal diff-gist saved should anyone be interested in the future.

It sounds like you got the setup you wanted anyway with the serial setup.
Using i2c would only shave off >1ms of latency in your setup, so it's not like you would notice.

Good to know! Ack! I only ventured down the i2c-road because I had read a few complaints about serial being "slow/laggy". Oh well. :)

Thanks again for all your help. I think this can be closed; I'd be happy to help test stuff with my keyboard (if I still have it) should anyone brings this up in the future.

[ishtob]

I am very interested in your "hacky" mod you did to get the RGB on the salve half. I am also working on a let's split and was hoping to use the i2C to transmit the RGB DI

[Lenbok]

Is this code on a branch somewhere that we could take a look?

[drashna]

IIRC, @That-Canadian had the rgb part working, IIRC. But issues with the animations.

[That-Canadian]

I do indeed have RGB working over I2C, no animations though. It's on my profile under the "Lets_Split_Eh" branch if anyone wants to looks, I'll be making a PR soon once I'm done testing it.

[Lenbok]

@That-Canadian Awesome, thanks, I'll take a look at that!

[Lenbok]

@That-Canadian Looks like you're doing a bunch of general split cleanup that'll knock off #1600 in there too, nice!

[Lenbok]

@mannkind @That-Canadian @ishtob Now that the initial static RGB over i2c is merged and working, maybe we can take a look at getting the animations going too? It seems that the animations can currently diverge between the left and right halves for two main reasons:

• Slight variation in underlying timer speeds between the two halves. (This could be addressed as @ahtn says by transmitting the timestamp over i2c)

• Variations in the times when each animation function gets to process the timer (I could imagine that if the master and slave have different amounts of work to do, this could be the dominating factor). Currently the animations tend to have pseudocode like this:

  if (timer_elapsed(last_timer) < pgm_read_byte(&RGBLED_RAINBOW_MOOD_INTERVALS[interval])) {
    return;
  }
  last_timer = timer_read();

The way last_timer is updated, every time a frame is late, all subsequent frames will be delayed by that amount and the error will accumulate over time. It seems that last_timer instead needs to increment at the expected frame rate only, so that globally the frames can occur at the right time. e.g.:

  if (timer_elapsed(last_timer) < pgm_read_byte(&RGBLED_RAINBOW_MOOD_INTERVALS[interval])) {
    return;
  }
  last_timer += pgm_read_byte(&RGBLED_RAINBOW_MOOD_INTERVALS[interval]);

(pseudocode, I'm not sure if it's valid in qmk to just increment timers like that or if there's a safe function to do that)