C (AS7 + gcc) pointers and "small data"

Go To Last Post
14 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Greetings Freaks -

 

Here is a situation that has been nagging at me for some time. This happens to be a function in the MC-written TWI module for Mega0/1 AVRs:

 

uint8_t TWI_MasterWrite(uint8_t slave_address,
			uint8_t *write_data,
			uint8_t bytes_to_write,
			uint8_t send_stop)

Now, suppose that I want to write two bytes. And those two bytes are #def'd constants. The first just happens to be a register address and the second is the data that goes into that register. My first inclination is to do:

 

uint8_t DataBuf[] = {CONTROL_REG, CONTROL_BYTE};
uint8_t RetVal = TWI_MasterWrite(DEVADDR,DataBuf,2,TRUE);

But, that seems wasteful with a lot of "just shuffling bytes around". IS this the preferred way to do it?

 

Its even worse when you do a 1 byte write (register address) and a 1 byte read (register contents). Then, you have pointers to 1 byte variables and it would seem that you have to create local variables to hold those #def'd constants rather than use the constants directly in the parameter list. Anything better when you are starting with a function that someone else has created?

 

Collateral question: I have seen this style of parameter listing in FatFS and now this library. I am not sure that I like it but I guess that I could get used to it. I can see a few advantages. Thoughts?

 

Thanks

Jim

 

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Remember #define is only a string substitution process. It doesn't instatiate anything. So yeah if you want a block of 2 bytes to pass you DO have to instantiate it somewhere so I see nothing wrong with your suggestion. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks. I understant, that. I was hoping that I could somehow substitute a constant in a parameter list where a pointer is defined. Guess that is maybe in the realm of C++.

 

Oh, well ...

 

Any thoughts about the parameter list style?

 

Thanks

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've seen that before Jim, it suggests setting up a IO_control block, (IOCB) that is a structure with the command, parameters or constants and buffer(s) as needed, then you call the function with a pointer to the IOCB, the io function knows how to read/write the structure.  In your case above, the struct has R/W command, slave address, buffer address, byte count, etc..  the advantage is the function call is the same regardless of how complex the i/o is, and returns a success or fail status, all other i/o is in the structure. 

 

I don't know if that is best practice for embedded micros.

 

Jim

 

 

 

 

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:

My first inclination is to do:

 

uint8_t DataBuf[] = {CONTROL_REG, CONTROL_BYTE};
uint8_t RetVal = TWI_MasterWrite(DEVADDR,DataBuf,2,TRUE);

But, that seems wasteful with a lot of "just shuffling bytes around". IS this the preferred way to do it?

 

Yes,  that looks fine.   Live with it.

 

There is a big advantage in writing a single "universal" function.   In fact,  I use

int8_t twi_master_trans(uint8_t slave, uint8_t *txbuf, int txn, uint8_t *rxbuf, int rxn);

You might write a helper function that simplifies your common calls e.g.

int8_t i2c_reg_val(uint8_t slave, uint8_t reg, uint8_t val);

Just use the style that makes you happy.    Only worry if you start running out of Flash or stack.    Most straightforward programs never approach memory limits.

 

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks!

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

that seems wasteful with a lot of "just shuffling bytes around". IS this the preferred way to do it?

Yes, I'm afraid so.

This is a common "difference in abstractions" between "higher level systems" and what we're used to in AVR-class systems.

 

Small microcontrollers tend to be programed with "stream-based" semantics, where the bottom-level primitive is "get/put single-byte", while bigger systems tend to be "file-based", where the bottom level is "read/write buffer", so you wind up with:

 

// microcontroller
void write(file, buffer, len) {
   for (len) {
     putc(file, *buffer++);
  }
}


// bigger system
void putc(file, c) {
   write(file, &c, 1);
}

On systems that are actually small and have byte-oriented peripherals, it's mostly a toss-up.  a low-level write() is likely to be more efficient for longer writes, and a low-level putc() is more efficient for very short writes.

Once you get to more complex peripherals (USB, Ethernet, DMA, actual disk files ... anything whose native implementation deals with more than one byte at a time), the low-level write() becomes MUCH better matched to reality, and the inefficiencies of implementation of "shifting bytes around" are dwarfed by the inefficiencies of very small transactions, anyway.)

(It's still annoying that "standard APIs" essentially fail to recognize the existence of single-byte peripherals :-( )

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'd be more concerned about the overhead of repeated calls to byte-oriented functions, e.g. when printing a long string. But I suppose we trust the compiler's optimiser to inline stuff where appropriate, rather than spending most of the time pushing and popping stuff to/from the stack.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

define a struct that holds all the fields you need and then make a const 'struct' and add that to the function call instead of all the separate fields.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thats not the issue.

 

In some places, the number of write bytes is fairly large, a dozen or more. In other places, it is only one byte. I just didn't like the one-byte case and having to use a pointer. I don't see that a struct changes anything. 

 

Besides, this is a 3rd party library from Microchip and I'd prefer not to mess with our over-lords.

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I suppose this is a "number of repetitions" thing. If you find yourself writing "Your first inclination" many more times than you'd like to, then write a small Facade function which simplifies the Microchip API.

 

My own tolerance limit may be quite low here - perhaps 3 times.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Jim,

 

So I guess this represents what you have now?

#include <avr/io.h>

#define FALSE 0
#define TRUE !FALSE
#define CONTROL_REG 0x2A
#define CONTROL_BYTE 0x55

uint8_t DataBuf[] = {CONTROL_REG, CONTROL_BYTE};

uint8_t TWI_MasterWrite(uint8_t addr, uint8_t * data, uint8_t len , uint8_t stop) { return 0; }

int main(void) {
	TWI_MasterWrite(123, DataBuf, 2, TRUE);
	while (1) {
	}
}

I think what you may be looking for is:

#include <avr/io.h>

#define FALSE 0
#define TRUE !FALSE
#define CONTROL_REG 0x2A
#define CONTROL_BYTE 0x55

uint8_t TWI_MasterWrite(uint8_t addr, uint8_t * data, uint8_t len , uint8_t stop) { return 0; }

int main(void) {
	TWI_MasterWrite(123, (uint8_t [2]){CONTROL_REG, CONTROL_BYTE}, 2, TRUE);
	while (1) {
	}
}

I guess it's a question of style but I like the clarity of the former ("Databuf" could even be a more meaningful name given the reader more clues as to what the sequence delivers like:

uint8_t PumpOff[] = {CONTROL_REG, CONTROL_BYTE};

then

TWI_MasterWrite(123, PumpOff, 2, TRUE);

Sure the former is more typing but I imagine the generated code will actually be an identical sequence (I can't test that bit as this all gets optimized away).

Last Edited: Tue. Oct 13, 2020 - 09:48 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

A helper function is pretty simple.

static int8_t i2cRegData(uint8_t slave, uint8_t reg, uint8_t data)
{
    uint8_t txbuf[2];
    txbuf[0] = reg;
    txbuf[1] = data;
    return twi_master_trans(slave, txbuf, 2, NULL, 0);
}

and there might be multiple calls to this helper e.g.

    i2cRegData(MCP23017, 0x0A, 0xA0); //BANK=1, SEQOP=1 if BANK0 (POR)
    i2cRegData(MCP23017, 0x05, 0xA0); //BANK=1, SEQOP=1 i.e. BYTE_MODE
    i2cRegData(MCP23017, IODIRA, (LCD_CMD_MASK | LCD_RESET | LCD_BLK) ^ 0xFF); //Output
    i2cRegData(MCP23017, IODIRB, 0xFF ^ 0xFF);                              //Output
    i2cRegData(MCP23017, OLATA, LCD_CMD_PORT = LCD_RESET);
    i2cRegData(MCP23017, OLATA, LCD_CMD_PORT = 0);
    i2cRegData(MCP23017, OLATA, LCD_CMD_PORT = LCD_RESET);

whereas this program only calls the full-fat sequence in one place e.g.

                twi_master_trans(MCP23017, txbuf, cnt, NULL, 0); //N strobes

Yes,  the helper will involve a bit of extra stack use and Flash.   But the the "calls" are smaller.

 

In practice high level I2C calls are never going to be time-critical.   But the HAL "universal" function has probably been written for the hardware.

 

Use whatever you are happier with.    You will find it easier to develop.

And for successors to maintain.

 

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the tutorial. Learned (well maybe "exposed to") a couple of new-to-me things today.

 

Really appreciate it. 

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!