Offset not being used

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

Guys I have a small routine that is not working.  Have a look below:

/*
 * Get a new cluster bit from the File Allocation Table
 */
int get_cluster_bit(struct ff_file * stream){


	char * ByteMap = safe_malloc (512);
	char * temp_ptr = ByteMap;



	int block = 0;
	int block_offset = 0;

	// read first FAT bit entry
	hardware_read_block (1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + stream->partition, 1, (char *)temp_ptr, 0);

	// search for a free bit in the FAT
	for(block = 0; (* ByteMap == 0xff); block++, ByteMap++){

		if((block % 512) == 0){

			// read next cluster
			block_offset++;
			hardware_read_block (1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + stream->partition + block_offset, 1, (char *)temp_ptr, 0);
		}
	}


	// retrieve free bit index
	char Bit = free_bit[(int)*ByteMap];

	// set FAT free bit
	* (temp_ptr + block) |= 1 << Bit;

	// update FAT table
	hardware_write_block (1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + block_offset + stream->partition, 1, (char *)temp_ptr, 0);

	int ret = (block * 8) + Bit + 2;

	safe_free(temp_ptr);
	return ret;
}

The part of interest is:

	// set FAT free bit
	* (temp_ptr + block) |= 1 << Bit;

it appears not to use the offset "block".  Any suggestions lads?

This topic has a solution.
Last Edited: Wed. Jul 3, 2019 - 02:43 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The first time entering the loop block = 0, then (block % 512) == 0 as well, so hardware_read_block() is called a second time before all the bytes of the first cluster are checked.  Not sure if this is intentional.

 

This code is a little complicated but looks valid.  You might be running into a compiler bug or side effects of compiler optimization.

// set FAT free bit
	* (temp_ptr + block) |= 1 << Bit;

 

To check for compiler errors and for debugging purposes only, unravel the complexity and see if it behaves as you expect.  Something like this:

 

// set FAT free bit
//   * (temp_ptr + block) |= 1 << Bit;

char *p = temp_ptr + block;
char original_value = *p;
char or_value = 1 << Bit;
char new_value = original_value | or_value;
*p = new_value;

 

Then step through the code with the debugger and check each line for weirdness.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
* (temp_ptr + block) |= 1 << Bit;

OK, give in, why the pointer maths when God invented arrays?..

temp_ptr[block] |= 1 << Bit;

There's no point needlessly obfuscating the code.

Last Edited: Sat. Jun 15, 2019 - 01:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not the issue, but:

(char *)temp_ptr

... why the cast of temp_ptr?  It's already a pointer to char.

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Actually the more I read that the more "upside down" it looks. Apart from the fact that you can treat the mallocation as an array then only ever change the index not the base pointer, with what you have temp_ptr remains fixed (for the free()) while ByteMap is varied. That just seems the wrong way round. Also try to be consistent in you variable naming, either you are using camel case (ByteMap) or you aren't (temp_ptr).

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

I'm using malloc because using arrays with "hardware_write_block" and "hardware_read_block" cause the system to fail.  I'm going to concentrate on correcting that error first.

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


clawson wrote:
temp_ptr remains fixed (for the free()) while ByteMap is varied.

I don't like that either; I thought that you had mutated ByteMap was the bug at first viewing. All that messing with ByteMap does obfuscate the actual bug however.

 

Copying the code into a decent editor and highlighting ByteMap makes the bug obvious:

 

 

ByteMap is continuously incremented in the loop and never reset back to (ByteMap's initial value) after readng the next block.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
char Bit = free_bit[(int)*ByteMap];

I think this line is suspect too.   Typecasting the address of a single byte, to the address of a 4 byte value (int, uint), usually requires the underlying to be aligned on the 4 byte boundary - which the incremented looped ByteMap++  param will not be.

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

rossh_af wrote:

char Bit = free_bit[(int)*ByteMap];

I think this line is suspect too.

 

If OP has written:

char Bit = free_bit[*(int*)ByteMap];

Then that would indeed be a bug and could cause a hardware exception , but what he wrote is OK. I'd go so far as to say the (int) cast is unnecessary.

 

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

Guys, as some of you have mentioned, why do I use Malloc. When I try to use an array it fails.  Now I have found why!  Have a look at the following:

 

/* Read/write data buffer.
 * It may receive data transferred from the device by the DMA. As the L1 data
 * cache won't notice when RAM is updated directly, the driver will invalidate
 * any matching cache lines.
 * May this buffer fail to be aligned on cache lines, cached changes to adjacent
 * variables would be lost at the time the dirty and shared cache lines were
 * invalidated.
 * Alternatively, we might consider allocating this buffer from a
 * non-cacheable memory region. */
// CACHE_ALIGNED_DDR static uint8_t data_buf[512ul];

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

Oh! Good spot. Did you already know the driver used DMA ? Was it documented well ?

 

In the world of PIC32 where I live now; we use __attribute__((coherent)) to specify a variable lies in the segment of memory (that you have) configured for cache coherency.

 

Does your processor support coherent memory regions or do you have to perform workarounds ?

 

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

Given the address and size of the array, you should be able to talk to the cache controller and invalidate the cache lines that have that area. That should be part of the API for your rtos.
Or as suggested above, you allocate an area of memory deemed non cacheable.

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

I'll have a look at it again tomorrow and let you guys know what the craic is.

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

Be sure to hit it with a shillelagh!

 

 

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

Kartman wrote:

Be sure to hit it with a shillelagh!

 

 

 

I will kartman

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

Every time someone mentions  "shillelagh" I'm reminded of a funny incident on RTE's Friday night "Late, Late Show" with Gay Byrne. He was telling some American (politically correct as it turned out) politician about how he'd been given a shillelagh. The interviewee asked "what's that then?" and so he explained it was a "stick we use for beating the fairies". The politician was apparently outraged and got very upset by this! :-)

 

"Two countries (or three in this case) divided by a common language" was perhaps never so true!

 

(for the non-Irish the "Fairies" are the "little folk" that have a lot of influence over luck/chance)

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

clawson wrote:

Every time someone mentions  "shillelagh" I'm reminded of a funny incident on RTE's Friday night "Late, Late Show" with Gay Byrne. He was telling some American (politically correct as it turned out) politician about how he'd been given a shillelagh. The interviewee asked "what's that then?" and so he explained it was a "stick we use for beating the fairies". The politician was apparently outraged and got very upset by this! :-)

 

"Two countries (or three in this case) divided by a common language" was perhaps never so true!

 

(for the non-Irish the "Fairies" are the "little folk" that have a lot of influence over luck/chance)

 

My father god rest his soul had one.

 

On a lighter note, my bugs all seem to be with system cache.

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

Fianawarrior wrote:

On a lighter note, my bugs all seem to be with system cache.

'all' is a bold statement!

Can't you, as a test, configure all memory regions as non-cacheable?

Probably run horrendoulsy slow, but just as a test.

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

MrKendo wrote:

Fianawarrior wrote:

On a lighter note, my bugs all seem to be with system cache.

'all' is a bold statement!

Can't you, as a test, configure all memory regions as non-cacheable?

Probably run horrendoulsy slow, but just as a test.

 

Getting there lads!

Could not be bother with anymore  coding today, Thanks lads.

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

so I was trying to use a DMA channel using my own data structures.  Problem solve so far.  I'll know more tomorrow.

Thanks lads for your help.  Maybe I need a shillelagh to beat the code into place?

 

Slán

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

ByteMap should have been reset in the if loop.

Anyway, still does not work.  I'll get back to this function tomorrow with a per of fresh eyes.

Last Edited: Tue. Jul 2, 2019 - 11:21 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Decided to re-write the function and low and behold it "LIVES, it LIVES".

Ah well.

 

/**
 *
 * @param stream - file stream pointer
 * @return new cluster bit
 */
__attribute((optimize("-O1")))int get_cluster_bit(struct ff_file * stream){


	char * bitMap = stream->cache_base;
	dma_hardware_read(1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + stream->partition_index, 1, stream->cache_base, 0);


	int byte = 0, sector = 0;

	for(byte = 0; *bitMap == 0xff; byte++, bitMap++){

		if(((byte % 512) == 0) && (byte != 0)){

			sector++;
			bitMap = stream->cache_base;

			dma_hardware_read(1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + stream->partition_index + sector, 1, stream->cache_base, 0);
		}
	}

	int bit = free_bit[(int)*bitMap];

	// write bit to target byte
	* bitMap |= 1 << bit;

	// write first FAT bit entry
	dma_hardware_write(1, data_to_cpu32(stream->Bios_Parameter_Block->ClusterHeapOffset) + stream->partition_index + sector, 1, stream->cache_base, 0);
	return ((byte * 8) + bit + 2);
}

 

Last Edited: Wed. Jul 3, 2019 - 02:36 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not enough comments ;-)