dsPIC33CH I2C code syntax problem (SOLVED)

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
Hi peeps,
I've just picked this up again after 6 months or so as I need to put a new PC together.

I've had it working a few times but not giving the expected logic output.
I've tried to comment it up somewhat to explain what I think it's doing or should be doing.

I'm attempting to write as a block of data two bytes, namely the (operation mode) register address & the register value.

If you guys could flame me with some blatently stupid errors I'd appreciate it.



main.c:
/**

  @File Name
    main.c

*/

/**
 PROJECT NAME: X1632-8Mhz_BNO055_Comms3

    
  Section: Included Files
*/
#include "xc.h"
#include "mcc_generated_files/system.h"
#include "mcc_generated_files/uart1.h"
#include "mcc_generated_files/i2c1.h"
#include <libpic30.h> //Defines __delay32();
#include <stdio.h> // Defines printf("");


int main(void)
{
    // initialise the device
    SYSTEM_Initialize();
    
/*    INITIALISES THE FOLLOWING
    PIN_MANAGER_Initialize();
    CLOCK_Initialize();
    INTERRUPT_Initialize();
    I2C1_Initialize();
    UART1_Initialize();
    INTERRUPT_GlobalEnable();
    SYSTEM_CORCONModeOperatingSet(CORCON_MODE_PORVALUES);
*/
    
    
 __delay32(5000);

          //  SYSTEMS USED & PROJECT AIMS
    printf("Explorer 16/32 v3 DM240001-3, ICD4, dsPIC33CH128MP508 PIM\n\r");
    printf("MPLABX v5.4, MCC v4.0.2, XC16 v1.60\n\r");
    printf("Project Name: X1632-8Mhz_BNO055_Comms3\n\r");
    printf("Simple write to BNO055 Comms via I2C\n\n\r");
    // This ^^ all works fine.
 /*
  * From                            To                  Switching time
  * CONFIGMODE              Any operation mode              7ms
  * Any operation mode          CONFIGMODE                  19ms
  */
    
#define CONFIGMODE          (00000000)        // default value = config mode
#define OPR_MODE            (00111101)        //Register 0x3D
#define ACCGYRO_CON         (00000101)        //Register Value 0x05
#define BNO55_I2C_ADDR      (00101000)        //Slave Device Address 0x28
  // In debug mode in MPLABX, when hovering the above shows as "Not recognised"

uint8_t counter;   
uint8_t *pdata;     // block to be sent, in this case the address of the register to write to, and the value to write.
uint8_t length;     // number of elements in pdata.
uint16_t address;   // Slave device address.
I2C1_MESSAGE_STATUS *pstatus;   //I2C returned status message.
 
 length = 2;        // number of bytes to transfer in the for loop.
 address = (BNO55_I2C_ADDR>>8); // Shift the value to the LSB as only writing 7 bit address.

while(pstatus != I2C1_MESSAGE_FAIL)     // After I2C1_MasterWrite is called this status is returned. The default is ENUMERATING  and so satisfys the test.
{
for (counter =0; counter < length ;counter++)   //counter to loop increment 'operation mode' Register & value index.
{
//   SET OP_MODE TO ACCGYRO
uint8_t data[2] = {OPR_MODE, ACCGYRO_CON};      //address of the OP_MODE register to write to, and the value to write.

pdata = &data[counter];     // transfer first byte of data.
I2C1_MasterWrite(*pdata, length, address, pstatus);
 __delay32(5000);
 
printf("I2C Message status = %08x \n\r",pdata);         //debug
 __delay32(5000);
}
}
 printf("I2C Message status = %c \n\r",pstatus);        //debug
 printf("Device Address = %08x \n\r",address);          //debug

}
/**
 End of File
*/
Any help would be appreciated.

Regards
GB
 

AlbertHall

Joined Jun 4, 2014
10,780
Mistake 1: using MCC. I never got MCC I2C code to work properly. I wrote some bit bang instead.

Mistake 2:
Line 5: #define BNO55_I2C_ADDR (00101000) //Slave Device Address 0x28
Followed by:
address = (BNO55_I2C_ADDR>>8); // Shift the value to the LSB as only writing 7 bit address.
When you shift 0x28 eight places to the right the answer you get is 0x00!

Then two possible mistakes because these may be OK depending what is in thos header files.
Possible Mistake 3: The value of 'address' is never used.
Possible Mistake 4: You check the value of 'pstatus' but I can't see any place where it gets a value assigned.
 

MrChips

Joined Oct 2, 2009
22,536
#define OPR_MODE (00111101) //Register 0x3D

#define is simply text substitution

use instead

#define OPR_MODE 0x3D

and in all such define statements.
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
Mistake 1: using MCC. I never got MCC I2C code to work properly. I wrote some bit bang instead.

Mistake 2:
Line 5: #define BNO55_I2C_ADDR (00101000) //Slave Device Address 0x28
Followed by:
address = (BNO55_I2C_ADDR>>8); // Shift the value to the LSB as only writing 7 bit address.
When you shift 0x28 eight places to the right the answer you get is 0x00!

Then two possible mistakes because these may be OK depending what is in thos header files.
Possible Mistake 3: The value of 'address' is never used.
Possible Mistake 4: You check the value of 'pstatus' but I can't see any place where it gets a value assigned.

Appreciate the reply, and.....
pstatus is returned when I2C1_MasterWrite is called.
I2C1.h Extract:
    [USER=120004]@Param[/USER]
        address - The address of the i2c peripheral to be accessed
    
    [USER=120004]@Param[/USER]
        length - The length of the data block to be sent
    
    [USER=120004]@Param[/USER]
        *pdata - A pointer to the memory location where received data will
                 be stored

    [USER=120004]@Param[/USER]
        *pstatus - A pointer to the status variable that the i2c driver
            updates during the execution of the message.

    @Returns
        I2C1_MESSAGE_STATUS
'address' is defined as a 16 bit unsigned integer, and so I'm thinking that the device address of 0x28 needs shifting to the right ?
or does an 8 bit address look the same in a 16 bit register and the 8 right hand side bits (i.e the MSB) get ignored?

0000000000101000
00101000
and therefore not need shifting at all?

Maybe I should just try it, it won't take a second. But if somebody would be kind enough to explain, that would be nice.

Regards
GB
 

MrChips

Joined Oct 2, 2009
22,536
Ok, thank you I've done this, I was thinking it had to be a binary literal.
Learn the difference between binary, decimal, and hexadecimal.
As far as the computer is concern it wants binary, always.
Different number representations such as binary, decimal, and hexadecimal are for confused humans.
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
@ MrChips
I know the difference between Binary, Decimal, & Hex, I'm just confused as to which one to use and when. In this case I used the binary version because the example did.

Trouble I've spotted now is that the Variables window & the intellisense debugging is showing the values in the variables when running in step-through, to be mixed up. The address is showing as the first block of data, and the length is showing the value of the address! Not sure what's going on there. Obviously needs some investigation.


@ Albert
" For this compiler at least a binary literal is written as 0b00111101 "

So that #define could be written as :
#define ACCGYRO_CON 0b00000101

Regards
GB
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
Almost Working Code:
  Section: Included Files
*/
#include "xc.h"
#include "mcc_generated_files/system.h"
#include "mcc_generated_files/uart1.h"
#include "mcc_generated_files/i2c1.h"
#include <libpic30.h> //Defines __delay32();
#include <stdio.h> // Defines printf("");


#define CONFIGMODE 0b00000000        // default value = config mode
#define OPR_MODE 0b00111101        //Register 0x3D
#define ACCGYRO_CON 0b00000101        //Register Value 0x05
#define BNO55_I2C_ADDR 0b00101000        //Slave Device Address 0x28
    
#define DEVICE_TIMEOUT 50
#define RETRY_MAX 100

int main(void)
{
    // initialise the device
    SYSTEM_Initialize();
    
/*    INITIALISES THE FOLLOWING
    PIN_MANAGER_Initialize();
    CLOCK_Initialize();
    INTERRUPT_Initialize();
    I2C1_Initialize();
    UART1_Initialize();
    INTERRUPT_GlobalEnable();
    SYSTEM_CORCONModeOperatingSet(CORCON_MODE_PORVALUES);
*/
    
    
 __delay32(50000);

          //  SYSTEMS USED & PROJECT AIMS
    printf("\n\n\r Explorer 16/32 v3 DM240001-3, ICD4, dsPIC33CH128MP508 PIM\n\r");
    __delay32(5000);
    printf("MPLABX v5.4, MCC v4.0.2, XC16 v1.60\n\r");
    __delay32(5000);
    printf("Project Name: X1632-8Mhz_BNO055_Comms3\n\r");
    __delay32(5000);
    printf("Simple write to BNO055 Comms via I2C  \n\n\r");
    __delay32(5000);
    __delay32(5000);
    
    
 /*
  * From                            To                  Switching time
  * CONFIGMODE              Any operation mode              7ms
  * Any operation mode          CONFIGMODE                  19ms
  */
      
uint8_t counter;    // Loop counter
uint8_t *pdata;     // block to be sent, in this case it's the address of the register to write to, and the value to write.
uint8_t length;     // number of elements in pdata.
uint16_t address;   // Slave device address.
I2C1_MESSAGE_STATUS *pstatus;   //I2C returned status message.
uint16_t    retryTimeOut, slaveTimeOut;


 length = 2;        // number of bytes to transfer in the for loop.
 address = (BNO55_I2C_ADDR); // Shifted (in the I2C1 TRB function) to the LSB as only writing 7 bit address.
 retryTimeOut = 0;
 slaveTimeOut = 0;
// I2C1_MESSAGE_STATUS status = I2C1_MESSAGE_PENDING;
 
 
while(pstatus != I2C1_MESSAGE_FAIL)     // After I2C1_MasterWrite is called this status is returned. The default is ENUMERATING  and so satisfys the test.
{
    //   SET OP_MODE(0x3D) TO ACCGYRO(0x05)
  uint8_t data[2] = {OPR_MODE, ACCGYRO_CON};      //address of the OP_MODE register to write to, and the value to write.
  pdata = data[0];     // Initialise the source of the data.
 
for (counter =0; counter < length ;counter++)   //counter to loop increment 'operation mode' Register & value index.
{
I2C1_MasterWrite(*pdata, length, address, pstatus);
__delay32(5000);
               // wait for the message to be sent or status has changed.
                while(pstatus == I2C1_MESSAGE_PENDING)
                {
                   __delay32(5000);


                    // timeout checking
                    // check for max retry and skip this byte
                    if (slaveTimeOut == DEVICE_TIMEOUT)
                        break;
                    else
                        slaveTimeOut++;
                }

printf("\n\n\r I2C Message status = %X \n\n\r",pstatus);     //debug
__delay32(5000);
printf("Device Address = %08x \n\n\r",address);         //debug
__delay32(5000);
printf("Data  = %08x \n\n\r",pdata);                    //debug
__delay32(5000);

pdata= data[1];

//    if (pstatus == I2C1_MESSAGE_COMPLETE)
//    break;
}
if (retryTimeOut == RETRY_MAX)
                    break;
                else
                    retryTimeOut++;
 
}
  while(1)
    {
      
    };
}

Salea_Plot_Address_No_Data.png

The above code gives me the (attached) output on the analyser, the device address is working fine. The register address and value are also sent and acknowledged, however the values are not correct, and although the vales are passed correctly the other parts i.e. transmission block building, and inserting into the transmission queue, seem to muddle up the correct values.

Anyone else come across this issue, and know what's going on?
 

AlbertHall

Joined Jun 4, 2014
10,780
for (counter =0; counter < length ;counter++) //counter to loop increment 'operation mode' Register & value index.
{ I2C1_MasterWrite(*pdata, length, address, pstatus);
This code executes the 'for' loop 'length' times. Within the loop I2C1_MasterWrite has length as a parameter and so presumably sends 'length' bytes of data.
Seems wrong to me.
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
This code executes the 'for' loop 'length' times. Within the loop I2C1_MasterWrite has length as a parameter and so presumably sends 'length' bytes of data.
Seems wrong to me.
Yes That was my intention, and I'm assuming (and correct me if I'm wrong) that the 'register address' and 'register value' total two bytes, I'm also assuming that after the first byte is sent, a restart is created to send the second byte. I thought that was what the logic analyser output is telling me with the 'ACK's ?
Which is why I thought it's the values being sent are wrong, assuming 0x00 is an acceptable value, but not what I want.
Regards
GB
 
Last edited:

AlbertHall

Joined Jun 4, 2014
10,780
Your analyser print shows only one I2C transaction (from start to stop). Your code should generate two such transactions. Can you find and show both transactions?
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
Your analyser print shows only one I2C transaction (from start to stop). Your code should generate two such transactions. Can you find and show both transactions?
The only evidence I have of the output is via the printf statement, as all the logic waveforms look the same (with the zero -zero waveforms). The printf statement outputs on my terminal show 0x3D and 0x05 being recorded in the pdata variable.

Question: Isn't the first transmission in the waveform the address (0x50 i.e. 0x28 <1) which has an 'ACK' the second part should have been the register address (0x3D) which also has an 'ACK' with the third part being the register value (0x05), isn't that a complete 'Write' function? Why should that happen twice?

Regards
GB
 
Last edited:

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
Because that is what your program does with that for loop. The logic analyser should capture the whole transaction if you set the sampling time long enough, say 10 seconds.
It creates that waveform 100 times (i.e. MAX_RETRY_VALUE ) then stops but they are all the same, with the '0x50-0x00-0x00' waveform.

Isn't the I2C1_MasterWrite function creating a 'restart' for the loop, just before the +0.5ms (i.e. the start of the register address byte ) & the +0.7ms (the start of the register value byte), or am I reading this wrong?
 

AlbertHall

Joined Jun 4, 2014
10,780
This line is wrong:
pdata = data[0]; // Initialise the source of the data.

data[0] is the value stored in location 0, not a pointer.
Should be pdata = data;
 

Thread Starter

GettinBetter

Joined Jun 22, 2018
32
This line is wrong:
pdata = data[0]; // Initialise the source of the data.

data[0] is the value stored in location 0, not a pointer.
Should be pdata = data;
Duly noted and corrected, thank you.

Updated 19-12-20:
/**
PROJECT NAME: X1632-8Mhz_BNO055_Comms3
  
  Section: Included Files
*/
#include "xc.h"
#include "mcc_generated_files/system.h"
#include "mcc_generated_files/uart1.h"
#include "mcc_generated_files/i2c1.h"
#include <libpic30.h> //Defines __delay32();
#include <stdio.h> // Defines printf("");


#define CONFIGMODE 0b00000000        // default value = config mode
#define OPR_MODE 0b00111101        //Register 0x3D
#define ACCGYRO_CON 0b00000101        //Register Value 0x05
#define BNO55_I2C_ADDR 0b00101000        //Slave Device Address 0x28
   
#define DEVICE_TIMEOUT 50
#define RETRY_MAX 100

int main(void)
{
    // initialise the device
    SYSTEM_Initialize();
   
/*    INITIALISES THE FOLLOWING
    PIN_MANAGER_Initialize();
    CLOCK_Initialize();
    INTERRUPT_Initialize();
    I2C1_Initialize();
    UART1_Initialize();
    INTERRUPT_GlobalEnable();
    SYSTEM_CORCONModeOperatingSet(CORCON_MODE_PORVALUES);
*/
   
   
__delay32(50000);

          //  SYSTEMS USED & PROJECT AIMS
    printf("\n\n\r Explorer 16/32 v3 DM240001-3, ICD4, dsPIC33CH128MP508 PIM\n\r");
    __delay32(5000);
    printf("MPLABX v5.4, MCC v4.0.2, XC16 v1.60\n\r");
    __delay32(5000);
    printf("Project Name: X1632-8Mhz_BNO055_Comms3\n\r");
    __delay32(5000);
    printf("Simple write to BNO055 Comms via I2C  \n\n\r");
    __delay32(5000);
    __delay32(5000);
   
   
/*
  * From                            To                  Switching time
  * CONFIGMODE              Any operation mode              7ms
  * Any operation mode          CONFIGMODE                  19ms
  */
     
uint8_t counter;    // Loop counter
uint8_t *pdata;     // block to be sent, in this case it's the address of the register to write to, and the value to write.
uint8_t length;     // number of elements in pdata.
uint16_t address;   // Slave device address.
I2C1_MESSAGE_STATUS *pstatus;   //I2C returned status message.
uint16_t    retryTimeOut, slaveTimeOut;


length = 2;        // number of bytes to transfer in the for loop.

address = (BNO55_I2C_ADDR); // Shifted (in the I2C1 TRB function) to the LSB as only writing 7 bit address.
retryTimeOut = 0;
slaveTimeOut = 0;

     //   SET OP_MODE(0x3D) TO ACCGYRO(0x05)
uint8_t data[]= {OPR_MODE, ACCGYRO_CON};      //address of the OP_MODE register to write to, and the value to write.

while(pstatus != I2C1_MESSAGE_FAIL)     // After I2C1_MasterWrite is called this status is returned. The default is ENUMERATING  and so satisfys the test.
{



  pdata = data;     // Initialise the source of the data.

for (counter = 0; counter < length ;counter++)   //counter to loop increment 'operation mode' Register & value index.
{
I2C1_MasterWrite(*pdata, length, address, &pstatus);
__delay32(5000);
               // wait for the message to be sent or status has changed.
                while(pstatus == I2C1_MESSAGE_PENDING)
                {
                   __delay32(5000);


                    // timeout checking
                    // check for max retry and skip this byte
                    if (slaveTimeOut == DEVICE_TIMEOUT)
                        break;
                    else
                        slaveTimeOut++;
                }

printf("\n\n\r I2C Message status = %X \n\n\r",pstatus);     //debug
__delay32(5000);
printf("Device Address = %08x \n\n\r",address);         //debug
__delay32(5000);
printf("Data  = %08x \n\n\r",*pdata);                    //debug
__delay32(5000);

pdata++;

}
if (retryTimeOut == RETRY_MAX)
                    break;
                else
                    retryTimeOut++;
if (pstatus == I2C1_MESSAGE_COMPLETE)
    break;

}
  while(1)
    {
     
    }
return 1;
}
/**
End of File
*
*/
So with a few other changes I get the following on terminal and logic analyser. As I mentioned before, the values at the variables are correct, and the address is correct. On terminal there are only two bytes of data (as expected) and the status shows 'message complete', so IMO the values aren't getting sent because they aren't going through the transmission block or message queue properly.... well maybe. It could be my crappy code.

Terminal_19-12-20.png
Salea_Plot_Address_No_Data_19-12-20.png
I'll continue to attempt debug, but if anyone has any suggestions I'd be glad to hear them.

Regards
GB
 
Last edited:
Top