This question is a bit of a long read to explain. Recently, I have been using Microchip’s XC8 C compiler with a PIC16F18875 board (Curiosity HPC). In this case, I used MCC (Microchip’s code configurator) to configure I2C and it is the first time that I have ever used MCC, but my question is not about the merits or demerits of MCC.

The generated code produces a function:

void I2C1_MasterWrite(
  uint8_t *pdata,
  uint8_t length,
  uint16_t address,
  I2C1_MESSAGE_STATUS *pflag)
{

that can be called to write to an I2C device. This function is working as I think it should and I am using a cheap I2C LCD as the device.

Notice that the function does not return a variable. Status of the transaction, however, is ascertainable by the *pstatus which points to a status variable that is updated during the transaction.

The variable is an enum type defined as follows:

typedef enum
{
  I2C1_MESSAGE_COMPLETE,
  I2C1_MESSAGE_FAIL,
  I2C1_MESSAGE_PENDING,
  I2C1_STUCK_START,
  I2C1_MESSAGE_ADDRESS_NO_ACK,
  I2C1_DATA_NO_ACK,
  I2C1_LOST_STATE
} I2C1_MESSAGE_STATUS;

So, error checking can be accomplished by examining I2C1_MESSAGE_STATUS after calling the I2C1_MasterWrite. OK fine, I'm getting to my question….

I have a function LCD_writexy that sends a string to the LCD at position x (0-15) and y (0-1) – all standard stuff. LCD_writexy() returns I2C1_MESSAGE_STATUS. The intention is to simply test if the I2C transaction was successful.

If LCD_writexy() is called with an out of bounds value, it returns without further action like this:

  // bounds check
  if ( (y > 1)  || (x > 15) ) {
     return (99);  // error return
   }

With this set up, this code works as I expect it to (e.g., code below writes '99' on the LCD):

  if( LCD_writexy(18,1,"oob")==99 )
  {
  LCD_writexy(0,1,"99");
  }

I would like to keep going with it like that but wonder how “kosher” it is to do so and that is my question. The only alternative that I can easily see is to add my own status values to the enum declaration. I am cautious about doing that because it is MCC generated code and I don’t want to uncover unanticipated effects.

Enum types annoy me sometimes because it seems like they are ints except when they are not. Any advice?

 

IMO You don't want to have non-I2C1_MESSAGE_STATUS codes in that enum declaration for clarity of the error generation function in your code. OOB is usually a binary fail-pass function so a display position failure reason code within the I2C operation communications status check seems out of place. How often will dynamically generated LCD positions be generated and can you bound check during that generation instead of during a display operation?

 

IMO You don't want to have non-I2C1_MESSAGE_STATUS codes in that enum declaration for clarity of the error generation function in your code. OOB is usually a binary fail-pass function so a display position failure reason code within the I2C operation communications status check seems out of place. How often will dynamically generated LCD positions be generated and can you bound check during that generation instead of during a display operation?

I see what you are saying about "clarity of the error function". While I was intuitively shying away from doing this ("The only alternative that I can easily see is to add my own status values to the enum declaration. I am cautious about doing that because it is MCC generated code and I don’t want to uncover unanticipated effects.", your reasoning is more to the point - that's not what the function is for and splicing into it seems distinctly "unkosher".

Your questions are more difficult to answer. I am just trying to gain familiarity and experience while utilizing an LCD. As general library usage, whenever I want to send something to the LCD screen as LCD_writexy(1,6,"Pos 6");, I will use it. If I double hit the "1", the error will occur and I don't see any practical way of checking the bounds at that entry point. If I were programming a number of LCD_writexy(xloc, yloc,"XXX"); yeah, I could bounds check when generating xloc and yloc and be done with it.

Also, while I wouldn't want to generalize to every possibility, but x and y are used to calculate the DDRAM address of display memory. As I recall, if there is no RAM at the address (there are only 80 bytes on the controller for display ram), or it is outside of the 32 bytes used for the display, nothing consequential other than it not being displayed will happen unless that extra ram is being used by the program (e.g., custom characters).

 

I see what you are saying about "clarity of the error function". While I was intuitively shying away from doing this ("The only alternative that I can easily see is to add my own status values to the enum declaration. I am cautious about doing that because it is MCC generated code and I don’t want to uncover unanticipated effects.", your reasoning is more to the point - that's not what the function is for and splicing into it seems distinctly "unkosher".

Your questions are more difficult to answer. I am just trying to gain familiarity and experience while utilizing an LCD. As general library usage, whenever I want to send something to the LCD screen as LCD_writexy(1,6,"Pos 6");, I will use it. If I double hit the "1", the error will occur and I don't see any practical way of checking the bounds at that entry point. If I were programming a number of LCD_writexy(xloc, yloc,"XXX"); yeah, I could bounds check when generating xloc and yloc and be done with it.

Also, while I wouldn't want to generalize to every possibility, but x and y are used to calculate the DDRAM address of display memory. As I recall, if there is no RAM at the address (there are only 80 bytes on the controller for display ram), or it is outside of the 32 bytes used for the display, nothing consequential other than it not being displayed will happen unless that extra ram is being used by the program (e.g., custom characters).

Not validating inputs and/or generation of data from invalid inputs for proper ranges and limits is one of the primary function/security failure points for code. A common way to bounds check a boolean value like (unsigned) 'y' is to only allow boolean values to the function, in that case a double "1" is still "1" and might flag a range warning during the compile if typed as a constant value in the program.

 

Not validating inputs and/or generation of data from invalid inputs for proper ranges and limits is one of the primary function/security failure points for code. A common way to bounds check a boolean value like (unsigned) 'y' is to only allow boolean values to the function, in that case a double "1" is still "1" and might flag a range warning during the compile if typed as a constant value in the program.

I'm not sure I understand. y is an uint8_t not boolean.
If I am writing a program using the library, say to display a temperature, I might type in the code line LCD_writexy(1,5,"Temp="); and then the temperature value converted to characters. I don't understand how what your saying regarding bounds checking applies at the point that I typing. The bounds checking I have now is in LCD_writexy() where x and y are checked for valid values. What am I missing?

 

Your bounds check for unsigned int y is >1 meaning valid values are [0 ..1] the boolean number value range so you don't need a uint8_t for y. This fact can be used to easily range limit the possible y values to the valid range of [0..1] for any possible set of unsigned integer inputs as characters during a conversion.

 

Your bounds check for unsigned int y is >1 meaning valid values are [0 ..1] the boolean number value range so you don't need a uint8_t for y. This fact can be used to easily range limit the possible y values to the valid range of [0..1] for any possible set of unsigned integer inputs as characters during a conversion.

Ahh ok I see what you are saying now. Of course that doesn't apply to x, but I understand now.