Passing different sized structs to same function...

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

Hi All,

 

I have an application where I'm storing presets of devices on a hot-swappable bus. We don't know the what device(s) will be used and how many presets we need to store for those devices until they are actually running.

 

My solution is to allocate a block of memory of different sized structures to store the presets in (to avoid malloc). Each structure has the same members, its just the array size for the presets varies.

 

struct Preset16					
{
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t presetActiveVar0[16];
	uint8_t presetActiveVar1[16];

};

struct Preset16 Preset16Bytes[5];

struct Preset24						
{
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t presetActiveVar0[24];
	uint8_t presetActiveVar1[24];

};

struct Preset24 Preset24Bytes[3];

When the devices initialize, they are assigned a preset block depending on the number of presets they need. If there are no more left of that size, say 16 bytes, they will claim a 24 byte slot. This just minimizes wasted memory (say if a 16 byte device tried to claim a 64 byte slot, that's 48 wasted bytes).
 

//Check if can fit in smallest preset size
	if (numOfPresetsNeeded <= 16)
	{
		//Check if any 16's are available
		for (arrayCounter = 0; arrayCounter <= 5; arrayCounter++)
		{
			if (Preset16Bytes[arrayCounter].presetDeviceAddress == 0)
			{
				//We found an open location. Store here.
				
				//Get preset number
				presetNum = convertToPresetNum(&Devices[deviceNum].deviceActiveKey, &Devices[deviceNum].deviceActiveVar);
				
				//Claim Preset Location for Device
				//Set Device Address in presetDeviceAddress
				Preset16Bytes[arrayCounter].presetDeviceAddress = Devices[deviceNum].deviceAddress;
				Preset16Bytes[arrayCounter].presetDeviceID = Devices[deviceNum].deviceID;
				
				//Store Current active variables in preset active Var
				Preset16Bytes[arrayCounter].presetActiveVar0[presetNum] = active0;
				Preset16Bytes[arrayCounter].presetActiveVar1[presetNum] = active1;
				
				//Link device to preset location
				Devices[deviceNum].ptrToPresets = &Preset16Bytes[arrayCounter];
				
				presetLocationFound = FOUND;
				break;
				
			}
		}
	}
	//Repeat for 24
	//Repeat for 32
	//Repeat for 64

Assigning this involves the same code over and over again (except I use the Preset24Byte struct, Preset32Bytestruct, etc), which would be more efficient if we could simply reuse the same code. What I'd like to do is is just be able to have the function accept a generic pointer to whatever struct address we give it, something like the pseudo code below, where sizeOfPresetArray would be there to bound the for loop to the structure's array size (5 for 16, 3 for 32, etc):

if (numOfPresetsNeeded <= 16)
{
    presetLocationFound = claimPresetLocation (&Preset16Bytes, sizeOfPresetArray);
}
else if ((numOfPresetsNeeded <=24) && (presetLocationFound != FOUND))
{
    presetLocationFound = claimPresetLocation (&Preset24Bytes, sizeOfPresetArray);
}
else if ((numOfPresetsNeeded <=32) && (presetLocationFound != FOUND))
{
    presetLocationFound = claimPresetLocation (&Preset32Bytes, sizeOfPresetArray);
}
...

Is this possible or is there a better way to do something like this?

Thank you all in advance!

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

Reorganize your data structures to allow flexible array members

or to pass the arrays in separately.

 

International Theophysical Year seems to have been forgotten..
Anyone remember the song Jukebox Band?

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

Thank you skeeve!

 

So for the second example, are you meaning something like this?

if (numOfPresetsNeeded <= 16)
{
    presetLocationFound = claimPresetLocation (Preset16Bytes[arrayCounter].presetDeviceAddress, Preset16Bytes[arrayCounter].presetDeviceID,...);
}

Then the function only needs to worry about uint8_t's like so?

uint8_t claimPresetLocation (uint8_t address, uint8_t ID)
{
    //Do stuff
}

 

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

Take a look at unions.  It's a way to share memory space but access member variables differently.  Just pay attention to byte alignment either with compiler options, #pragma's, or manually aligning variables.

 

Something like this:

struct Preset16					
{
	uint8_t size;               // assign to sizeof(struct Preset16)
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t presetActiveVar0[16];
	uint8_t presetActiveVar1[16];

};

struct Preset24						
{
    uint8_t size;               // assign to sizeof(struct Preset24)
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t presetActiveVar0[24];
	uint8_t presetActiveVar1[24];

};

union Preset
{
    struct Preset16 p16;
    struct Preset24 p24;
}

union Preset presetArray[5];    // allocate the shared storage here

int PresetFoo(union Preset *pPreset);       // example function prototype

 

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

But doesn't the union thing suffer the same? You still need multiple branches of code to handle pPreset[n]->p16.XXXX and pPreset[n]->p24.XXX and so on.

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

Yes, but the memory storage itself is shared in a mutual exclusive and efficient way.  There is a single array where each entry is either a struct Preset16 or struct Preset24, so no wasted space.

 

Use a switch statement on the struct size (or alternately call it struct type):

 

struct Preset16
{
    uint8_t size;               // assign to sizeof(struct Preset16)
    uint8_t presetDeviceAddress;
    uint8_t presetDeviceID;
    uint8_t presetActiveVar0[16];
    uint8_t presetActiveVar1[16];

};

struct Preset24
{
    uint8_t size;               // assign to sizeof(struct Preset24)
    uint8_t presetDeviceAddress;
    uint8_t presetDeviceID;
    uint8_t presetActiveVar0[24];
    uint8_t presetActiveVar1[24];

};

union Preset
{
    struct Preset16 p16;
    struct Preset24 p24;
};

union Preset presetArray[5];    // allocate the shared storage here - single efficient array

int PresetFoo(union Preset *pPreset);       // example function prototype

void Foo(void)
{
    union Preset preset;

    preset.p16.size = sizeof(struct Preset16);
    preset.p16.presetDeviceAddress = 12;
    preset.p16.presetDeviceID = 34;
    // ...

    PresetFoo(&preset);

    preset.p24.size = sizeof(struct Preset24);
    preset.p24.presetDeviceAddress = 56;
    preset.p24.presetDeviceID = 78;
    // ...

    PresetFoo(&preset);
}

int PresetFoo(union Preset *pPreset)
{
    int iReturn;

    switch (pPreset->p16.size) {
    case sizeof(struct Preset16):
        // do struct Preset16 stuff here

        iReturn = (int)pPreset->p16.size;
        break;

    case sizeof(struct Preset24):
        // do struct Preset24 stuff here

        iReturn = (int)pPreset->p24.size;
        break;

    default:
        // handle invalid or unknown preset types here

        iReturn = -1;
        break;
    }

    return iReturn;
}

 

edit: fixed compiler errors with the pseudo code

Last Edited: Wed. Aug 8, 2018 - 05:07 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you ScottMN! This seems to solve a problem but also creates some.

In the original question, I was more trying to figure out how to make the coding more efficient since I already accepted the fact that this application would use a ton of SRAM and EEPROM. This would cut down on RAM usage but makes the code more complex.

 

It's looking like the best solution I have with this current arrangement is to just declare the struct's globally and access them in the functions directly like I was doing. I also realized that trying to link the Preset address with a Device (in the second code block of the OP) runs into the same problem as passing the different structs to the function.

 

malloc and flexible arrays might be the way to go, but I'm a little scared of malloc on an AVR (likely will use a 324PA...trying to only spend 550 bytes or so on presets). This would actually not waste any space (aside from padding) since the Device's preset needs will be fixed, so we could then use 12, 16, 18 byte presets, exactly what the device needs. We wouldn't have to worry about accessing members either because a device with 16 presets will never try to store a preset in the 22nd preset slot.

 

It seems like a combo of skeeve's flexible array members and ScottMN's union would make this task as efficient as possible in terms of code complexity, performance and resource usage. Is this correct?

 

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

I've not looked closely at your allocator.

I do not know your allocator requirements.

If you never give any memory back,

just pick the next however many bytes you need.

It can be from an array of uint8_t.

On an AVR, you need not worry about alignment.

 

I'm suggesting that  instead of two array members with one-byte entries,

you have one array member with two-byte entries.

That will allow you to go from multiple types distinguished by sizes of arrays

to having one type with a flexible array member.

Pass the function a pointer to the struct and the number of entries in its array (singular).

International Theophysical Year seems to have been forgotten..
Anyone remember the song Jukebox Band?

Last Edited: Wed. Aug 8, 2018 - 07:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

How about a level of indirection to make the Preset struct identical:

 

struct Preset
{
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t *presetActiveVar0;
	uint8_t *presetActiveVar1;
};

struct Preset Preset[8];

Obviously your memory allocator must now allocate blocks of either 16 or 24 bytes for each presetActiveVar and also initialise the pointers. At first sight this looks quite managable actually.

 

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

skeeve wrote:

I've not looked closely at your allocator.

I do not know your allocator requirements.

If you never give any memory back,

just pick the next however many bytes you need.

It can be from an array of uint8_t.

On an AVR, you need not worry about alignment.

 

I was planning on just declaring them as global structs so that memory would always be claimed for this purpose. I've read in many places that the best practice on uC's is to just claim what you need. There can only be 8 devices on the bus at one time, and most of the devices will be small in preset needs. So the 32 and 64 byte presets are just there in case, since I intend this to be able to be future proof. The only time I was planning to give any memory back is if a device was never going to be used on the bus again, I was going to write a function that would allow those preset slots to be unpopulated. That should be easy enough to do, just zero the entire structure.

 

skeeve wrote:

I'm suggesting that  instead of two array members with one-byte entries,

you have one array member with two-byte entries.

That will allow you to go from multiple types distinguished by sizes of arrays

to having one type with a flexible array member.

Pass the function a pointer to the struct and the number of entries in its array (singular).

 

I think I get this now and it seems like the best option AND it seems like I could avoid malloc by declaring it globally too, huh? There's no reason I can't pack the actives into 1 variable. Then this would also allow me to link the device to the preset like I was trying to do originally.

Thank you for your help! 

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

N.Winterbottom wrote:

How about a level of indirection to make the Preset struct identical:

 

struct Preset
{
	uint8_t presetDeviceAddress;
	uint8_t presetDeviceID;
	uint8_t *presetActiveVar0;
	uint8_t *presetActiveVar1;
};

struct Preset Preset[8];

Obviously your memory allocator must now allocate blocks of either 16 or 24 bytes for each presetActiveVar and also initialise the pointers. At first sight this looks quite managable actually.

 

Thank you N.Winterbottom!

Ya, this looks like another good option. Admittedly, I'm not great at programming so this may be a bit harder for me to pull off, but now I have a few options thanks to you guys!