11

I am doing some work in embedded C with an accelerometer that returns data as a 14 bit 2's complement number. I am storing this result directly into a uint16_t. Later in my code I am trying to convert this "raw" form of the data into a signed integer to represent / work with in the rest of my code.

I am having trouble getting the compiler to understand what I am trying to do. In the following code I'm checking if the 14th bit is set (meaning the number is negative) and then I want to invert the bits and add 1 to get the magnitude of the number.

int16_t fxls8471qr1_convert_raw_accel_to_mag(uint16_t raw, enum fxls8471qr1_fs_range range) {
  int16_t raw_signed;
  if(raw & _14BIT_SIGN_MASK) {
    // Convert 14 bit 2's complement to 16 bit 2's complement
    raw |= (1 << 15) | (1 << 14); // 2's complement extension
    raw_signed = -(~raw + 1);
  }
  else {
    raw_signed = raw;
  }
  uint16_t divisor;
  if(range == FXLS8471QR1_FS_RANGE_2G) {
    divisor = FS_DIV_2G;
  }
  else if(range == FXLS8471QR1_FS_RANGE_4G) {
    divisor = FS_DIV_4G;
  }
  else {
    divisor = FS_DIV_8G;
  }

  return ((int32_t)raw_signed * RAW_SCALE_FACTOR) / divisor;
}

This code unfortunately doesn't work. The disassembly shows me that for some reason the compiler is optimizing out my statement raw_signed = -(~raw + 1); How do I acheive the result I desire?

The math works out on paper, but I feel like for some reason the compiler is fighting with me :(.

secretformula
  • 6,256
  • 2
  • 30
  • 53
  • You could try `if(raw & _14BIT_SIGN_MASK) raw_signed = (int16_t)(raw | 0xC000);` – Weather Vane Dec 03 '15 at 20:59
  • There is no language "embedded C". And left shifting a signed value that cannot be represented in the variable is undefined behaviour. – too honest for this site Dec 03 '15 at 21:01
  • 1
    If you turn up all the compiler warnings, do you get any relevant diagnostics? If you compile with optimization turned off does the program work as intended? – John Bollinger Dec 03 '15 at 21:02
  • @Olaf I was trying to make it clear that I was working with an embedded system so I have certain constraints / limitations John and Weather; ill give it a try – secretformula Dec 03 '15 at 21:03
  • The "embedded" tag makes that clear already. Yet that does not imply any specific restrictions per se. And for the question as given, it does not matter anyway. Note that it is not the compiler fighting, but you likely have a problem with integer promotion and/or implementation defined and undefined behaviour. – too honest for this site Dec 03 '15 at 21:05
  • 1
    And "my code doesn't work is not a **specific** problem statement. Unless you provide proof, the compiler is likely right optimising your code as it does. Use a debugger and debugging-friendly optimisations, e.g. gcc `-Og`. – too honest for this site Dec 03 '15 at 21:09
  • The statement `raw_signed = -(~raw + 1)` is equivalent to `raw_signed = raw`. That's why the compiler optimizes it out. – user3386109 Dec 03 '15 at 21:10
  • You could try `-(int16_t)((~raw + 1) & 0x3fff)`. That is, perform a *14-bit* sign inversion, convert the (positive) result to `int16_t`, and negate it. Alternatively, casting your existing expression to `int16_t` before negating might also help. – John Bollinger Dec 03 '15 at 21:11
  • Doesn't any 2's complement number only need a sign bit extension? – Weather Vane Dec 03 '15 at 21:12
  • @user3386109, the compiler is can perhaps perform `raw_signed = raw` instead of `raw_signed = -(~raw + 1)`, but that doesn't mean it can omit the assignment altogether. – John Bollinger Dec 03 '15 at 21:13
  • 1
    @WeatherVane Yes, but the current C standard declares that any operation that directly manipulates the sign bit is undefined behavior, since it won't work on a one's complement machine, or a sign-magnitude machine. – user3386109 Dec 03 '15 at 21:15
  • 1
    @JohnBollinger Yes, but the other branch of the `if` also does the same assignment, and the rest of the code only uses `raw_signed`, so the optimized assembly will not contain an explicit assignment. – user3386109 Dec 03 '15 at 21:17
  • 1
    @JohnBollinger: It will likely use the same register for both, not needing a move. Anyway, `1<<15` is already UB for 16 bit `int` (note the tags). No need to do further research what else could have gone wrong. – too honest for this site Dec 03 '15 at 21:19
  • @user3386109 my example was not manipulating the sign bit, it was unsigned and manipulated before casting to signed. – Weather Vane Dec 03 '15 at 21:19
  • Why so complicated and not just `raw_signed = raw; if ( raw & (1U << 13)) raw_signed |= 0xC000;`? – too honest for this site Dec 03 '15 at 21:22
  • @Olaf, because that produces undefined behavior, as it boils down to assigning an out-of-range value to a signed integer. – John Bollinger Dec 03 '15 at 21:26
  • @JohnBollinger that code worked! Thanks alot, do you want to put that into an answer I can accept? I'm trying to understand exactly why my code didnt work. If I wanted to do in the exact form that I had originally I would have needed to cast to a `int32_t` prior to performing the negation? – secretformula Dec 03 '15 at 21:27
  • @JohnBollinger how can any 14-bit signed or unsigned value be out of range of a 16-bit value? – Weather Vane Dec 03 '15 at 21:28
  • @JohnBollinger: Please elaborate. IIRC, it is implementation defined, but will work on AVR. – too honest for this site Dec 03 '15 at 21:32
  • @WeatherVane && Olaf, I find I must amend and qualify my remark. `raw_signed |= 0xC000`, produces *implementation-defined* behavior when `int` is 16 bits wide, because then `raw_unsigned | 0xc000` has type `unsigned int` and value outside the range of `int`. I acknowledge that I am making a logical leap by assuming 16-bit `int` in the target environment, but that seems a clean way to explain the observed compiler behavior. – John Bollinger Dec 03 '15 at 21:45
  • @WeatherVane I was wrong, it's **implementation defined behavior**, not **undefined behavior**. The relevant section is 6.3.1.3 which says: *"When a value with integer type is converted to another integer type [...], if the value can be represented by the new type, it is unchanged. [...] Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised."* The unsigned value `raw | 0xC000` is too big for a 16-bit signed int, so the cast to `int16_t` is implementation defined. – user3386109 Dec 03 '15 at 21:46
  • To be clear: When code reaches `return ((int32_t)raw_signed`, do you expect `raw_signed` to have a value in the range `[-8192 ... +8191]`? – chux - Reinstate Monica Dec 03 '15 at 22:26

5 Answers5

13

Converting the 14 bit 2's complement value to 16 bit signed, while maintaining the value is simply a metter of:

int16_t accel = (int16_t)(raw << 2) / 4 ;

The left-shift pushes the sign bit into the 16 bit sign bit position, the divide by four restores the magnitude but maintains its sign. The divide avoids the implementation defined behaviour of an right-shift, but will normally result in a single arithmetic-shift-right on instruction sets that allow. The cast is necessary because raw << 2 is an int expression, and unless int is 16 bit, the divide will simply restore the original value.

It would be simpler however to just shift the accelerometer data left by two bits and treat it as if the sensor was 16 bit in the first place. Normalising everything to 16 bit has the benefit that the code needs no change if you use a sensor with any number of bits up-to 16. The magnitude will simply be four times greater, and the least significant two bits will be zero - no information is gained or lost, and the scaling is arbitrary in any case.

int16_t accel = raw << 2 ;

In both cases, if you want the unsigned magnitude then that is simply:

int32_t mag = (int32_t)labs( (int)accel ) ;
Clifford
  • 76,825
  • 12
  • 79
  • 145
  • 1
    Before the UB police pipe up, I realise that strictly this is not guaranteed to work where 2's complement is not the signed encoding, but in the real world it works. In embedded systems you would expect to know how your hardware behaves and performance is often more critical than portability to some hypothetical machine. – Clifford Dec 03 '15 at 22:01
  • 2
    @chux : Thanks - fixed. Your two-step solution may be better for clarity. Personally I really wouldn't bother restoring to 14-bit magnitude scaling; it is an unnecessary step. – Clifford Dec 03 '15 at 22:06
  • A disadvantage of "... shift the accelerometer data left by two bits and treat it as if the sensor was 16 bit in the first place. " is with `int16_t mag = abs( accel ) ;` that may try to do `abs(0x8000)` – chux - Reinstate Monica Dec 03 '15 at 22:47
  • @chux : Thanks for the spot, changed to use `labs()` and `int32_t`, though `abs()` would still work on a 32 bit target. – Clifford Dec 04 '15 at 11:15
  • There's no need to call the UB police. However, here comes the implementation-defined behavior police (IDBP). Since `raw` is unsigned, the left shift is well-defined, no UB. And the conversion to `int16_t` is well-defined, given that both systems use two's complement with same endianess. The right shift of a negative number after that is not UB but implementation-defined: it could translate either to logical shift or arithmetic shift. This will cause problems in case of arithmetic shift! You'll have to dodge this somehow, like for example masking the final result with `& 0xBFFF`. – Lundin Dec 04 '15 at 13:48
  • @Lundin : An *arithmetc shift* is precisely what is required here; it is a logical shift that would cause a problem. Good point though, and strengthens the argument to left align the data to 16 bit in any case. Many hardware devices and ADC peripherals do this an an option in hardware in any case. If you have a high sample rate and real-time deadlines, the less unnecessary manipulation of the the data the better. – Clifford Dec 04 '15 at 13:55
  • @Clifford Ah yeah, the other way around :) Anyway, it shows that the shifts are problematic. One solution could be to do something semi-obscure like this: `int16_t left = (int16_t)(raw << 2); if(left < 0) { result = -(int16_t) ((uint16_t)-left >> 2) } else { result = left >> 2; }` – Lundin Dec 04 '15 at 15:17
  • 1
    @Lundin : To ensure it is arithmetic, simply divide by 4. The compiler will almost certainly generate an arithmetic-shift operation if the instruction set allows. Edited accordingly. – Clifford Dec 04 '15 at 22:08
  • @Lundin: `intX_t` are **guaranteed** to be 2s complement by the standard. – too honest for this site Dec 06 '15 at 15:30
6

I would do simple arithmetic instead. The result is 14-bit signed, which is represented as a number from 0 to 2^14 - 1. Test if the number is 2^13 or above (signifying a negative) and then subtract 2^14.

int16_t fxls8471qr1_convert_raw_accel_to_mag(uint16_t raw, enum fxls8471qr1_fs_range range) 
{
  int16_t raw_signed = raw;
  if(raw_signed >= 1 << 13) {
    raw_signed -= 1 << 14;
  }

  uint16_t divisor;
  if(range == FXLS8471QR1_FS_RANGE_2G) {
    divisor = FS_DIV_2G;
  }
  else if(range == FXLS8471QR1_FS_RANGE_4G) {
    divisor = FS_DIV_4G;
  }
  else {
    divisor = FS_DIV_8G;
  }

  return ((int32_t)raw_signed * RAW_SCALE_FACTOR) / divisor;
}

Please check my arithmetic. (Do I have 13 and 14 correct?)

uncleO
  • 7,997
  • 2
  • 20
  • 37
  • Thanks for this code, your arithmetic was correct. Just for the exercise I ended up using the code John provided above to understand the necessary manipulations / casts but this code was valid too (and produced much less head scratching)! – secretformula Dec 03 '15 at 21:29
  • 1
    This answer is optimal, as it does not depend on the native signed type being 2's compliment, nor does it depend on `raw_signed` being exactly 16 bit. – dbush Dec 03 '15 at 21:55
  • @secretformula Most programming time is spent maintaining and debugging code. Over the long run, it always pays off to avoid "head-scratching" type code! – uncleO Dec 03 '15 at 21:57
  • 2
    Well, `politically optimal`. – user3528438 Dec 03 '15 at 22:02
  • @dbush : It is "portable" not "optimal". *Optimal* usually implies highest possible performance, and code that accommodates hypothetical hardware rather then being tailored to the actual hardware is sub-optimal. This question is tagged *embedded*, so will be full of code specific to the target, so optimising this to the target would make sense. Moreover, where are these one's complement or sign+magnitude machines? For an optimal *and* portable solution, conditional compilation would be a better approach. – Clifford Dec 03 '15 at 22:19
1

Supposing that int in your particular C implementation is 16 bits wide, the expression (1 << 15), which you use in mangling raw, produces undefined behavior. In that case, the compiler is free to generate code to do pretty much anything -- or nothing -- if the branch of the conditional is taken wherein that expression is evaluated.

Also if int is 16 bits wide, then the expression -(~raw + 1) and all intermediate values will have type unsigned int == uint16_t. This is a result of "the usual arithmetic conversions", given that (16-bit) int cannot represent all values of type uint16_t. The result will have the high bit set and therefore be outside the range representable by type int, so assigning it to an lvalue of type int produces implementation-defined behavior. You'd have to consult your documentation to determine whether the behavior it defines is what you expected and wanted.

If you instead perform a 14-bit sign conversion, forcing the higher-order bits off ((~raw + 1) & 0x3fff) then the result -- the inverse of the desired negative value -- is representable by a 16-bit signed int, so an explicit conversion to int16_t is well-defined and preserves the (positive) value. The result you want is the inverse of that, which you can obtain simply by negating it. Overall:

raw_signed = -(int16_t)((~raw + 1) & 0x3fff);

Of course, if int were wider than 16 bits in your environment then I see no reason why your original code would not work as expected. That would not invalidate the expression above, however, which produces consistently-defined behavior regardless of the size of default int.

John Bollinger
  • 121,924
  • 8
  • 64
  • 118
  • Thanks for helping me understand my error. I ended up going with Cliffords answer because it was more concise. Thank you again! – secretformula Dec 03 '15 at 22:38
  • Disagree with "Also if int is 16 bits wide...." paragraph. The `-(~raw + 1)` will all be done with 16-bit unsigned math. The result will be in the range of `int`, so no problem for not all 16-bit combinations are present in `raw`, just 2^14 of them. – chux - Reinstate Monica Dec 03 '15 at 22:43
1

Assuming when code reaches return ((int32_t)raw_signed ..., it has a value in the [-8192 ... +8191] range:

If RAW_SCALE_FACTOR is a multiple of 4 then a little savings can be had.

So rather than

int16_t raw_signed = raw << 2; 
raw_signed >>= 2;

instead

int16_t fxls8471qr1_convert_raw_accel_to_mag(uint16_t raw,enum fxls8471qr1_fs_range range){
  int16_t raw_signed = raw << 2;
  uint16_t divisor;
  ...
  // return ((int32_t)raw_signed * RAW_SCALE_FACTOR) / divisor;
  return ((int32_t)raw_signed * (RAW_SCALE_FACTOR/4)) / divisor;
}
chux - Reinstate Monica
  • 113,725
  • 11
  • 107
  • 213
0

To convert the 14-bit two's-complement into a signed value, you can flip the sign bit and subtract the offset:

int16_t raw_signed = (raw ^ 1 << 13) - (1 << 13);
D Krueger
  • 2,326
  • 13
  • 12