I'm having a discussion with some people I work with. I reviewed some code they wrote where a sample is acquired from an ADC. The ADC outputs (ADS1220) digital samples as 24bit signed samples. Three bytes, (B0,B1,B2), are read in a row from a SPI buffer where the first byte is the most significant byte. The sample has to be converted to an unsigned 16-bit value.
I am claiming that the conversion has to be done like this:
signed long Convert24BitSignedTo32BitSigned(unsigned char b1, unsigned char b2, unsigned char b3)
{
signed long r = 0;
unsigned char b0 = 0xff;
if ((b1 & 0x80) != 0) r = r | ((signed long) (b0 << 24));
r = r | ((signed long) (b1 << 16));
r = r | ((signed long) (b2 << 8));
r = r | ((signed long) b3);
return r;
}
u32 u32sample;
u32sample.byte.B2 = <read first byte>
u32sample.byte.B1 = <read first byte>
u32sample.byte.B0 = <read first byte>
u32sample.u32val = (unsigned long) (Convert24BitSignedTo32BitSigned(B2, B1, B0) + 8388608);
u32sample.u32val = u32sample.u32val >> 8
This is the way it's currently done in the code:
u32sample.byte.B2 = <read first byte>
u32sample.byte.B1 = <read first byte>
u32sample.byte.B0 = <read first byte>
u32sample.u32val = u32sample.u32val >> 7
The type u_long is defined as:
typedef union {
unsigned long u32val;
struct {
unsigned char B0;
unsigned char B1;
unsigned char B2;
unsigned char B3;
} byte;
struct {
unsigned int low;
unsigned int high;
} uint;
} u32;
The size of an int is 16bit.
Converting 24bit signed sample to unsigned 16bit sample
Started by ●May 10, 2015
Reply by ●May 10, 20152015-05-10
Reply by ●May 10, 20152015-05-10
>I'm having a discussion with some people I work with. I reviewed some >code they wrote where a sample is acquired from an ADC. The ADC outputs >(ADS1220) digital samples as 24bit signed samples. Three bytes,(B0,B1,B2), are>read in a row from a SPI buffer where the first byte is the most >significant byte. The sample has to be converted to an unsigned 16-bitvalue.>[...snip...] If I understand this correctly, I think you are working too hard. A long is a signed 32 bit, correct? long v = ( B0 << 24 ) | ( B1 << 16 ) | ( B2 << 8 ); You now have your value in the upper 24 bits and the sign bit of B0 is the sign bit of your long. You don't even need to grab B2 if you want to truncate. unsigned int r = (unsigned int) ( ( ( v >> 16 ) + 0x8000 ) & 0xFFFF ); If you want a rounded value, replace 'v' with '( v + 0x8000 )'. When you right shift the v, the sign bit will be carried along. Add the bias, then just keep the low order two bytes. See the $10000 DAC thread about a discussion of maybe adding dithering. Ced --------------------------------------- Posted through http://www.DSPRelated.com
Reply by ●May 10, 20152015-05-10
On Sun, 10 May 2015 12:24:38 -0500, "Cedron" <103185@DSPRelated> wrote:>long v = ( B0 << 24 ) | ( B1 << 16 ) | ( B2 << 8 );Could also be v = ( B0 << 24 ) + ( B1 << 16 ) + ( B2 << 8 ); But that's not the most important point. Using explicit placement of the individual bytes ...>typedef union { > unsigned long u32val; > struct { > unsigned char B0; > unsigned char B1; > unsigned char B2; > unsigned char B3; > } byte; > struct { > unsigned int low; > unsigned int high; > } uint; >} u32;... will produce incorrect results in a big-endian architecture. Using shift operations allows the compiler to accommodate endianness automatically. http://en.wikipedia.org/wiki/Endianness
Reply by ●May 10, 20152015-05-10
>On Sun, 10 May 2015 12:24:38 -0500, "Cedron" <103185@DSPRelated> wrote: > >>long v = ( B0 << 24 ) | ( B1 << 16 ) | ( B2 << 8 ); > >Could also be > > v = ( B0 << 24 ) + ( B1 << 16 ) + ( B2 << 8 ); > >But that's not the most important point. Using explicit placement of >the >individual bytes ...[...snip...]>... will produce incorrect results in a big-endian architecture. Using >shift operations allows the compiler to accommodate endianness >automatically. > >http://en.wikipedia.org/wiki/EndiannessQuite true on both counts. I am pretty sure an 'or' and an 'add' take the same number of clock cycles, so no difference there. For the truncated version, I think my previous answer was also working a little too hard. This should do the trick: unsigned int r = ( ( B0 << 8 ) | B1 ) ^ 0x8000; I've omitted the casting references of (unsigned int) in front of B0 and B1. I still prefer the 'or' over 'add' as a convention when doing bit operations. Toggling the sign bit is equivalent to adding the bias. Ced --------------------------------------- Posted through http://www.DSPRelated.com
Reply by ●May 10, 20152015-05-10
On Sun, 10 May 2015 13:55:22 -0500, "Cedron" <103185@DSPRelated> wrote:>Quite true on both counts. I am pretty sure an 'or' and an 'add' take the >same number of clock cycles, so no difference there.Depends upon the CPU. Use whichever is more efficient.>For the truncated version, I think my previous answer was also working a >little too hard. This should do the trick: > >unsigned int r = ( ( B0 << 8 ) | B1 ) ^ 0x8000; > >I've omitted the casting references of (unsigned int) in front of B0 and >B1.I noticed later-on that either the union is backwards or the order of B0...B3 is backwards.>typedef union { > unsigned long u32val; > struct { > unsigned char B0; > unsigned char B1; > unsigned char B2; > unsigned char B3; > } byte; > struct { > unsigned int low; > unsigned int high; > } uint; >} u32;According to the union, B0 and B1 form the "low" uint and B2 and B3 form the "high" uint. If that's the case, then the equation needs to be: long v = ( B2 << 24 ) | ( B1 << 16 ) | ( B0 << 8 ); and your new truncated equation needs to be: unsigned int r = ( ( B2 << 8 ) | B1 ) ^ 0x8000; Either that, or the union needs to be rearranged: typedef union { unsigned long u32val; struct { unsigned char B3; unsigned char B2; unsigned char B1; unsigned char B0; } byte; struct { unsigned int low; unsigned int high; } uint; } u32; Endianness cautions still apply.
Reply by ●May 10, 20152015-05-10
On 2015-05-10 20:00, Greg Berchin wrote:> On Sun, 10 May 2015 12:24:38 -0500, "Cedron" <103185@DSPRelated> wrote: > >> long v = ( B0 << 24 ) | ( B1 << 16 ) | ( B2 << 8 ); > > Could also be > > v = ( B0 << 24 ) + ( B1 << 16 ) + ( B2 << 8 ); > > But that's not the most important point. Using explicit placement of the > individual bytes ... > >> typedef union { >> unsigned long u32val; >> struct { >> unsigned char B0; >> unsigned char B1; >> unsigned char B2; >> unsigned char B3; >> } byte; >> struct { >> unsigned int low; >> unsigned int high; >> } uint; >> } u32; > > ... will produce incorrect results in a big-endian architecture. Using > shift operations allows the compiler to accommodate endianness > automatically.Not only, but the compiler can arrange things as it pleases, so the struct content could be completely shuffled or padded or else. bye, pg> http://en.wikipedia.org/wiki/Endianness >-- piergiorgio
Reply by ●May 10, 20152015-05-10
On Sun, 10 May 2015 08:11:07 -0700, Mauritz Jameson wrote:> I'm having a discussion with some people I work with. I reviewed some > code they wrote where a sample is acquired from an ADC. The ADC outputs > (ADS1220) digital samples as 24bit signed samples. Three bytes, > (B0,B1,B2), are read in a row from a SPI buffer where the first byte is > the most significant byte. The sample has to be converted to an unsigned > 16-bit value. > > I am claiming that the conversion has to be done like this: > > signed long Convert24BitSignedTo32BitSigned(unsigned char b1, unsigned > char b2, unsigned char b3) > { > signed long r = 0; > unsigned char b0 = 0xff; > > if ((b1 & 0x80) != 0) r = r | ((signed long) (b0 << 24)); > r = r | ((signed long) (b1 << 16)); > r = r | ((signed long) (b2 << 8)); > r = r | ((signed long) b3); > return r; > }It looks like the intent of this is to be nice and portable, which it probably is. Furthermore, with a good optimizing compiler it shouldn't be much slower, and may well be faster, than the current method. I see two problems: first, ANSI-compatibility says that a long should be _at least_ 32 bits, but it doesn't say that it _must_ be 32 bits. I suspect that on a 64-bit machine, a long is 64 bits, which kind of blows your sign-extension line out of the water. A better way to do this would be to set: signed long r; if ((b1 & 0x80) == 0) r = 0; else r = 0 - 0x1000000; then carry on with OR-ing in the bytes. Note, however, that this still has the defect that it only works on machines that use 2's complement notation and that overflow silently. Today that's ubiquitous, but you never know when you'll find an edge case.> u32 u32sample; > > u32sample.byte.B2 = <read first byte> > u32sample.byte.B1 = <read first byte> > u32sample.byte.B0 = <read first byte> > > u32sample.u32val = (unsigned long) (Convert24BitSignedTo32BitSigned(B2, > B1, B0) + 8388608);I would find this more clear if the 8388608 were replaced by 0x800000.> u32sample.u32val = u32sample.u32val >> 8 > > This is the way it's currently done in the code:Hmm. Mostly looks workable.> u32sample.byte.B2 = <read first byte> > u32sample.byte.B1 = <read first byte> > u32sample.byte.B0 = <read first byte> > > > u32sample.u32val = u32sample.u32val >> 7 > > The type u_long is defined as: > > typedef union { > unsigned long u32val; > struct { > unsigned char B0; unsigned char B1; unsigned char B2;unsigned char> B3; > } byte; > struct { > unsigned int low; unsigned int high; > } uint; > } u32; > > > The size of an int is 16bit.OK. So, this code isn't portable. Maybe that's an issue, maybe not. I do see a honkin' big problem, though: your two pieces of code are going to come up with markedly different answers, each and every time. If there's a problem with the way the current implementation scales the data and behaves when the ADC is returning negative values, then maybe you're fixing it. If the current code works right and you're seeking to make it cleaner, then know that you'll be breaking it. -- Tim Wescott Wescott Design Services http://www.wescottdesign.com
Reply by ●May 10, 20152015-05-10
"Cedron" <103185@DSPRelated> writes:>>I'm having a discussion with some people I work with. I reviewed some >>code they wrote where a sample is acquired from an ADC. The ADC outputs >>(ADS1220) digital samples as 24bit signed samples. Three bytes, > (B0,B1,B2), are >>read in a row from a SPI buffer where the first byte is the most >>significant byte. The sample has to be converted to an unsigned 16-bit > value. >> > > [...snip...] > > If I understand this correctly, I think you are working too hard. > > A long is a signed 32 bit, correct? > > long v = ( B0 << 24 ) | ( B1 << 16 ) | ( B2 << 8 ); > > You now have your value in the upper 24 bits and the sign bit of B0 is the > sign bit of your long. You don't even need to grab B2 if you want to > truncate. > > unsigned int r = (unsigned int) ( ( ( v >> 16 ) + 0x8000 ) & 0xFFFF ); > > If you want a rounded value, replace 'v' with '( v + 0x8000 )'. > > When you right shift the v, the sign bit will be carried along. Add the > bias, then just keep the low order two bytes. > > See the $10000 DAC thread about a discussion of maybe adding dithering. > > CedCed, I like your overall approach but I would much rather see the C99 <stdint.h> used, e.g., int32_t and uint16_t, instead of long and unsigned int, respectively. -- Randy Yates Digital Signal Labs http://www.digitalsignallabs.com
Reply by ●May 10, 20152015-05-10
[...snip...]> >Depends upon the CPU. Use whichever is more efficient. >'or' is lower level than 'add', so if there is a difference, the best guess would be that the 'or' would be faster. [...snip...]> >Either that, or the union needs to be rearranged: > > typedef union { > unsigned long u32val; > struct { > unsigned char B3; > unsigned char B2; > unsigned char B1; > unsigned char B0; > } byte; > struct { > unsigned int low; > unsigned int high; > } uint; > } u32; > >Endianness cautions still apply.Since the data is coming in from a device, there is no reason to presume that the device architecture and the cpu architecture are consistent. The OP specified that B0 was the high order byte. Endianness is always a pain in the rear. On your revised struct definition I think you forgot to reverse the low & high in the second part. I avoided using the struct definition in my answers as it is not necessary, endian dependent, and probably slower (less optimizable). Ced --------------------------------------- Posted through http://www.DSPRelated.com
