Author Topic: Should multiple handles be used on a single physical I2C channel?  (Read 4593 times)

Offline tdrobnak

  • Newbie
  • *
  • Posts: 28
    • View Profile
After reading the "uTasker - I2C Support, Demo and Simulation V1.4" document, I have two questions concerning uTasker task scheduling after an I2C read and write.:
Given the following information:
   There are two devices on the I2C bus: ALTITUDE_SENSOR and LED_DRIVER

   1. If one physical I2C channel is being used on the microcontroller that both ALTITUDE_SENSOR and LED_DRIVER are connected to, would more than one call be needed for fnOpen():?

void
configureI2CInterface (void)
{
    /*~~~~~~~~~~~~~~~~~~~~~~~*/
    I2CTABLE    tI2CParameters;
    /*~~~~~~~~~~~~~~~~~~~~~~~*/

    tI2CParameters.Channel = OUR_I2C_CHANNEL;
    tI2CParameters.usSpeed = 100;   // 100k
    tI2CParameters.Rx_tx_sizes.TxQueueSize = 64;    // transmit queue size
    tI2CParameters.Rx_tx_sizes.RxQueueSize = 64;    // receive queue size
    tI2CParameters.Task_to_wake = TASK_ALTITUDE_SENSOR;                // task to wake on transmission

    if((I2CAltitudeSensorID = fnOpen(TYPE_I2C, FOR_I_O, &tI2CParameters)) != NO_ID_ALLOCATED)
    {   // open the channel with defined configurations
        g_I2CAltitudeSensorIDIsActive = TRUE;
    }

    tI2CParameters.Task_to_wake = LED_DRIVER;                // task to wake on transmission

    if((I2CLedDriverID = fnOpen(TYPE_I2C, FOR_I_O, &tI2CParameters)) != NO_ID_ALLOCATED)
    {   // open the channel with defined configurations
        g_I2CLedDriverIDIsActive = TRUE;
    }

Then when a write takes place it is of the form:
   For ALTITUDE_SENSOR:
      bufferToWrite[] = { 0xc0, 0xfd, 0x0b };
      fnWrite(I2CAltitudeSensorID, (unsigned char *)bufferToWrite, sizeof(bufferToWrite));
      bufferToRead[] = {1, ALTITUDE_SENSOR_ADDR, TASK_ALTITUDE_SENSOR};
      fnRead(I2CAltitudeSensorID, (unsigned char *)bufferToRead, 0);
   
   For LED_DRIVER:
      bufferToWrite[] = { 0xe8, 0x00, 0x55 };
      fnWrite(I2CAltitudeSensorID, (unsigned char *)bufferToWrite, sizeof(bufferToWrite));
      bufferToRead[] = {1,LED_DRIVER_ADDR, TASK_LED_DRIVER};
      fnRead(I2CLedDriverID, (unsigned char *)bufferToRead, 0);
      
By setting up two handles: I2CAltitudeSensorID, I2CLedDriverID uTasker can wake the appropriate task when data is written and read.
   2. If what I proposed in question 1 is not true, how would I set up uTasker to make active the task when data is written and read given only one I2CPortID handle?

Right now I have one handle for both devices called I2CPortID.  The problem is that uTasker seems to not activate the task to handle fnRead() after the data has been read unless a breakpoint is inserted after the fnRead().  When a breakpoint is inserted after the fnRead(), the the proper I2C task will handle the buffer, otherwise the task will not become active.

Any ideas of how I should proceed?

Offline mark

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 3234
    • View Profile
    • uTasker
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #1 on: November 19, 2019, 08:38:34 PM »
Hi

Each physical I2C interface is opened once and has a unique handle.

When handling reception from multiple slaves on the same bus I recommend having a single I2C task that is woken each time a read has completed. Since all commands to start the read and all read receptions takes place in the same order (FIFO) it is quite simple to know where a reception has been received from, even if multiple reads were queued.

It is not advised to use different tasks for reception (although this could be configured) if multiple reads can be initiated at the same time and each read has a different "owner" (the task scheduled when its complete read is ready) because it may not be possible to guarantee that the tasks are scheduled in the order that the receptions were completed (and are in the queue) and they may read the wrong data out of the queue.

What I tend to do is use a state event machine with a simple management queue which knows in which order reads were initiated (and thus in which order they will thus be received in, and the number of bytes each read expects). Each time there is an I2C message of the next expected length available its reason/content is also known and so it can be accurately processed and removed from the management queue, etc. etc.

Generally there is no need to use any "owner" for transmissions - unless there is a need to do if for timing reasons; such as to know when a transmission has physically completed on the bus.

Regards

Mark

Offline tdrobnak

  • Newbie
  • *
  • Posts: 28
    • View Profile
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #2 on: November 20, 2019, 05:34:23 PM »
Thank you for your feedback, Mark.  The task, TASK_LED_DRIVER, does not use and fnRead() functions; it only uses fnWrite() and assumes the data was transferred.  The task TASK_ALTITUDE_SENSOR needs to read the altitude and temperature from the sensor, so it uses both fnRead() and fnWrite() commands.  Therefore, there should not be any confusion on which task was commanding the read, but  I think I have identified the source of the problem I am having.

Looking over the uTasker code, I think there is the possibility of a missed wake up owner of fnRead() task:
   1. A fnRead() takes place in TASK_ALTITUDE_SENSOR and changes the ucState to RX_ACTIVE and TX_ACTIVE in file kinetis_LPI2C.h line 358 of function fnTxI2C:
           I2C_tx_control[Channel]->ucState |= (RX_ACTIVE | TX_ACTIVE);
   2. The uTaskerStateChange(ptrRxControl->wake_task, UTASKER_ACTIVATE) does not happen until all bytes are transferred into the I2CPortID message queue in the interrupt handler function, fnI2C_Handler(), file kinetis_LPI2C.h:
   static void fnI2C_Handler(KINETIS_LPI2C_CONTROL *ptrLPI2C, int iChannel) // general LPI2C interrupt handler
   {
       I2CQue *ptrTxControl = I2C_tx_control[iChannel];
   
       if ((ptrLPI2C->LPI2C_MIER & ptrLPI2C->LPI2C_MSR) & LPI2C_MSR_SDF) {  // stop condition has completed
           fnLogEvent('S', ptrLPI2C->LPI2C_MSR);
           ptrLPI2C->LPI2C_MIER = 0;                                        // disable all interrupt sources
           WRITE_ONE_TO_CLEAR(ptrLPI2C->LPI2C_MSR, LPI2C_MSR_SDF);          // clear interrupt flag
           if ((ptrTxControl->ucState & RX_ACTIVE) != 0) {
               I2CQue *ptrRxControl = I2C_rx_control[iChannel];
               ptrRxControl->msgs++;
               if (ptrRxControl->wake_task != 0) {                          // wake up the receiver task if desired
                   uTaskerStateChange(ptrRxControl->wake_task, UTASKER_ACTIVATE); // wake up owner task
               }
           }
           else {
               if (ptrTxControl->wake_task != 0) {
                   uTaskerStateChange(ptrTxControl->wake_task, UTASKER_ACTIVATE); // wake up owner task since the transmission has terminated
               }
           }
           ptrTxControl->ucState &= ~(TX_WAIT | TX_ACTIVE | RX_ACTIVE);     // interface is idle
What I think is happening is that while the fnRead() communication on the physical I2C channel 0 is taking place, a fnWrite() command is called in task TASK_LED_DRIVER.  Calling fnWrite() will set the ptrTxControl->ucState to TX_ACTIVE and clear RX_ACTIVE before the " if ((ptrTxControl->ucState & RX_ACTIVE) != 0)" conditional is executed in the interrupt handler fnI2C_Handler().  If this happens, then the uTaskerStateChange(ptrRxControl->wake_task, UTASKER_ACTIVATE) statement will not happen.  This will cause the TASK_ALTITUDE_SENSOR not to wake up and handle the data read from the I2C in the I2CPortID buffer.

I have found that if I disable the TASK_LED_DRIVER, the returned data from the fnRead()s in TASK_ALTITUDE_SENSOR are handled, otherwise, the data is not handled because the TASK_LED_DRIVER is not scheduled to wake up.  By disabling TASK_LED_DRIVER, I can see that it does affect the wake up scheduling of TASK_ALTITUDE_SENSOR.  I checked the address of both where uState is set, line 358 in file kinetis_LPI2C.h, function fnTxI2C() :
       if ((ucAddress & 0x01) != 0) {                                       // reading from the slave
(358)         I2C_tx_control[Channel]->ucState |= (RX_ACTIVE | TX_ACTIVE);
           ptI2CQue->I2C_queue.chars -= 3;
           fnLogEvent('g', (unsigned char)(ptI2CQue->I2C_queue.chars));
           I2C_rx_control[Channel]->wake_task = *ptI2CQue->I2C_queue.get++; // enter task to be woken when reception has completed
           if (ptI2CQue->I2C_queue.get >= ptI2CQue->I2C_queue.buffer_end) {
               ptI2CQue->I2C_queue.get = ptI2CQue->I2C_queue.QUEbuffer;     // handle circular buffer
           }
       }
       else {
           I2C_tx_control[Channel]->ucState &= ~(RX_ACTIVE);
           I2C_tx_control[Channel]->ucState |= (TX_ACTIVE);                 // writing to the slave
           ptI2CQue->I2C_queue.chars -= (ptI2CQue->ucPresentLen + 1);       // the remaining queue content
           fnLogEvent('h', (unsigned char)(ptI2CQue->I2C_queue.chars));
       }
And where it is checked, line 135 in file kinetis_LPI2C.h, function fnI2C_Handler():
   static void fnI2C_Handler(KINETIS_LPI2C_CONTROL *ptrLPI2C, int iChannel) // general LPI2C interrupt handler
   {
       I2CQue *ptrTxControl = I2C_tx_control[iChannel];
   
       if ((ptrLPI2C->LPI2C_MIER & ptrLPI2C->LPI2C_MSR) & LPI2C_MSR_SDF) {  // stop condition has completed
           fnLogEvent('S', ptrLPI2C->LPI2C_MSR);
           ptrLPI2C->LPI2C_MIER = 0;                                        // disable all interrupt sources
           WRITE_ONE_TO_CLEAR(ptrLPI2C->LPI2C_MSR, LPI2C_MSR_SDF);          // clear interrupt flag
(135)          if ((ptrTxControl->ucState & RX_ACTIVE) != 0) {
               I2CQue *ptrRxControl = I2C_rx_control[iChannel];
               ptrRxControl->msgs++;
               if (ptrRxControl->wake_task != 0) {                          // wake up the receiver task if desired
                   uTaskerStateChange(ptrRxControl->wake_task, UTASKER_ACTIVATE); // wake up owner task
               }
           }
           else {
               if (ptrTxControl->wake_task != 0) {
                   uTaskerStateChange(ptrTxControl->wake_task, UTASKER_ACTIVATE); // wake up owner task since the transmission has terminated
               }
           }
           ptrTxControl->ucState &= ~(TX_WAIT | TX_ACTIVE | RX_ACTIVE);     // interface is idle

Both "I2C_tx_control[Channel]->ucState" and "ptrTxControl->ucState" point to the same location (0x200001CF) using the segger j-link debugger.  This tells me that it is possible that ucState can be changed before it is handled.  Was this the intention of uTasker I2C design?  If not, could you provide a solution (fix) that allows an I2C read to be handled even if another task is executing a I2C write?  My thought on a work around right now is to queue up all I2C reads and writes from the two tasks and handle them in one task that will allow only one fnRead() or fnWrite() command at a time until the task that commanded the read or write is woken.

Please let me know you thoughts.  I appreciate your feedback.

Offline mark

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 3234
    • View Profile
    • uTasker
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #3 on: November 21, 2019, 12:32:53 AM »
Hi

Reads and writes are queued and so adding a write while a read in is process should not have any impact on the read.

Note that when a read is started the state is set to
I2C_tx_control[Channel]->ucState |= (RX_ACTIVE | TX_ACTIVE);

Then note that when a write is started the generic I2C driver layer (i2c_drv.c) will copy the write to the output queue:

rtn_val = fnFillBuf(&ptI2CQue->I2C_queue, ptBuffer, Counter);

and then initiate it ONLY when the channel is idle:

            if ((ptI2CQue->ucState & (TX_WAIT | TX_ACTIVE | RX_ACTIVE)) == 0) { // if idle we can initiate transmission
                fnTxI2C(ptI2CQue, channel);                              // initiate transmission
            }


Therefore the fact of queuing a write during a read transfer simply puts the write to the queue (without affecting the in-progress read in any way).

What I can't exclude is a problem in the LPI2C driver itself since it was added a long time after the standard I2C driver (in Kinetis project) and is only available in a small number of Kinetis parts (less well used/verified). So taking a quick look I immediately see that your version is not the same as mine (yours is pre-24.3.2019 check-in), which means that it probably makes sense to ensure you are using the present version in case such behavior may have been improved since then.

I have attached my file - there is no mention of a fix in its header (added new features) but the idea is that the RX_ACTIVE flag can only be changed after a read transfer has terminated and the owner task is scheduled each time the final expected byte has been put into the I2C Rx buffer. I didn't see that this could be missed but I do question that (in your version) the code that you show is executed only on a stop condition detection (a read immediately followed by a write tends to be performed with a repeated start and then there is no stop bit (??)). Therefore the latest version does look more realistic in this respect....

Regards

Mark





Offline tdrobnak

  • Newbie
  • *
  • Posts: 28
    • View Profile
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #4 on: November 21, 2019, 04:55:39 PM »
Hi Mark,

Thank you for identifying the difference in version I am using and a later one that you have coded to handle the repeated start condition.  Replacing the kinetis_LPI2C.h file with the newer one you provided fixed the missed uTaskerStateChange(ptrRxControl->wake_task, UTASKER_ACTIVATE).  My application is now working with both TASK_ALTITUDE_SENSOR and TASK_LED_DRIVER.

I did run into one compilation error on the kinetis_LPI2C.h file.  It looks like a new macro is used: WRITE_ONE_TO_CLEAR_INTERRUPT(ptrLPI2C->LPI2C_MSR, LPI2C_MSR_SDF, irq_id).  This one does not exist in my code version, so I replaced it with WRITE_ONE_TO_CLEAR(ptrLPI2C->LPI2C_MSR, LPI2C_MSR_SDF) and the code now compiles and executes with the desired result.

This leads me to the question of if it would be a good idea to update my version of uTasker to the latest in the GIT repository or do you have a specific version that you would recommend.  It might be a little effort on my part to merge my code into the latest version of uTasker, but if this will fix some behind the scene driver issues and not cause what has been working to fail, I will do it.  This missed scheduled task activation issue has cost me a few days of lost time while I try to understand the uTasker code and provide detailed enough observations for some assistance.  If you think using your a later version than uTasker_1.4.12.2019.02.28 for the Kinetis KE15Z processor then please let me know.

Thank you.

Tom

Offline mark

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 3234
    • View Profile
    • uTasker
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #5 on: November 22, 2019, 12:47:46 AM »
Tom

The aim is to be retain backward compatibility in the framework so generally it should be always possible to pull the latest version to get newest bug fixes and additional features. Pulling the latest version will not cause code size increases due to new features since these first need to be enabled (config.h and app_hw_xxx.h in the uTaskerV1.4 can be used to see new defines).

It is not possible to guarantee 100% due to the large possibilities (many boards, processors and options/features) but in case of anything not working post a report and switch back to an earlier check-out to continue working.

Regards

Mark


Offline tdrobnak

  • Newbie
  • *
  • Posts: 28
    • View Profile
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #6 on: November 22, 2019, 08:53:59 PM »
Thank you, Mark for your recommendation on pulling the latest version.  I am new to GIT since we use SourceGear Vault at the company I work for.  I have found that GIT does make it very easy to update uTasker by doing a "git pull" on the latest commit.  My project directory is not affected by the pull since it is in the Untracked files.  Only the updated uTasker files will be replaced.

Tom

Offline mark

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 3234
    • View Profile
    • uTasker
Re: Should multiple handles be used on a single physical I2C channel?
« Reply #7 on: November 22, 2019, 11:47:31 PM »
Tom

I found GIT to be a game changer for me. It is possible to have one repository for the framework and then a repository for each individual project in the framework. This allows all projects to share the framework (and its repository with capability to switch between each individual version at will) and each project directory to have its own such independent control too.

This I find both very convenient (history, change documentation, backup) and efficient.

Once a project is completed one can either commit the total code base to a dedicated, frozen project repository (to be sure of the complete code base) or else the framework check-in point noted so that it can be exactly recreated from the framework repository if ever needed. Any framework bug fixes that are made are valid for all projects and so each project can also be rebuilt at any time in the future to potentially benefit from such.

Regards

Mark