I feel like I have never written code before

[admiralk]

I am working on what, at this point, is a simple adding machine; 6 buttons +1, +3, +5, -5, -3, -1. It worked fine, until I tried to display a negative sign. The problem is in one simple line of code, the if statement.

Code: [Select]

displayNumber += 1;
Serial.print("Number = ");
Serial.println(displayNumber);
if(displayNumber >= 0)
flags &= ~(1 << FLAGNEG);

Serial.print("After check, number = ");
Serial.println(displayNumber);
Serial.println(flags, BIN);serial output:

Code: [Select]

Number = -4
After check, number = -4
1
// This snippet starts here
Number = -3
After check, number = -3
0
I am pretty sure that

Code: [Select]

if(-3 >= 0) should evaluate to false, and not change the flags bit. I am either overlooking something extremely simple, or there is something very wrong elsewhere.  I could really use a second set of eyes to tell me I have gotten really stupid blind, or give me an idea of where else to look.

[ataradov]

What is the type of displayNumber?

Also, are you sure that flags is properly set to begin with? What is FLAGNEG?

[Bud]

Is your displayNumber of unsigned type?

[hendorog]

If it was me I'd be putting a print statement inside the if statement.

Then when you do that and spend half an hour working out that you forgot to put the braces around it, then you can consider adding the braces as a matter of routine.

[admiralk]

If it was me I'd be putting a print statement inside the if statement.

Then when you do that and spend half an hour working out that you forgot to put the braces around it, then you can consider adding the braces as a matter of routine.

I did not forget, and as a matter of course, one of the first things I ruled out was whether the Arduino or VM wanted to suddenly be picky about that. As a matter of routine, I do not add unneeded braces

displayNumber is an int, it holds the number that is to be displayed (3 digits at this point, but will be larger).

Code: [Select]

int displayNumber = 0; // Number to displayI should have mentioned that the display is made up of 7 segment displays that I designed, but for here are equivalent to regular 7 segment displays otherwise.

flags is a "software register", for lack of a better term.

Code: [Select]

#define FLAGNEG 0 // 1 = negative number
// flags.1 =
// flags.2 =
// flags.3 =
// flags.4 =
// flags.5 =
// flags.6 =
// flags.7 =
uint8_t flags = 0x00;
I do not have positive numbers turning negative, it is only negative ones turning positive when displayNumbers is added to. In case it helps, here is what the button press events look like, minus the print code.

Code: [Select]

if(digitalRead(keyPin1)) // Key 1
{
displayNumber += 1;
if(displayNumber >= 0)
flags &= ~(1 << FLAGNEG);

digit.buildNumber(displays, digits, displayNumber);

digitalWrite(shiftPin, 0);

writeToDisplay();

digitalWrite(shiftPin, 1);
_delay_ms(400);
}
else if(digitalRead(keyPin3)) // Key 6
{
displayNumber -= 1;
if(displayNumber < 0)
flags |= (1 << FLAGNEG);

digit.buildNumber(displays, digits, displayNumber);

digitalWrite(shiftPin, 0);

writeToDisplay();

digitalWrite(shiftPin, 1);
_delay_ms(400);
}
I only check if the flag bit needs to be changed when displayNumber is changed.

Is your displayNumber of unsigned type?

yes, or it would not show negative in the print statements. It counts right, -3 + 1 = -2, but because the flag bit gets changed, it does not display the negative sign.

[hendorog]

I just meant that when you add the print statement it is a common error to forget to add the braces. Then you have two bugs instead of one and more confusion.
I've done that too many times so now I add the braces always.

[admiralk]

I always added them when I started. I leave the blank line after to get my attention that there is something changing now. It is much cleaner IMHO.

In this case, the fact that the flag changes, means that the if statement had to evaluate true for some reason.  I am much more prone to writing -3 = 0 instead of  - 3 ==0, which i did somewhere else. curse these bad eyes and my poor typing abilities.

[NorthGuy]

In this case, the fact that the flag changes, means that the if statement had to evaluate true for some reason.

Or "flags" could have changed somewhere else.

[admiralk]

Bingo!

Code: [Select]

void writeToDisplay()
{
usedDigits = digit.numberSize(displayNumber);

for(uint8_t i = 0; i < digits; i++)
{
if(usedDigits <= i)
{
if(flags & (1 << FLAGNEG))
{
digit.writeDigit(dataPin, clockPin, digit.minus);
flags &= ~(1 << FLAGNEG); // Needs to be turned back on after loop?
}
else
{
digit.writeDigit(dataPin, clockPin, digit.blank);
}
}
else
digit.writeDigit(dataPin, clockPin, digit.digit[displays[i]]);
}
}
I even made a comment about it. I forgot I clear the bit to avoid adding multiple negative signs if there are more digits. After I made that change, I fried a display (bad soldering), and completely forgot about it. I could not remember I changed the flag in a function, until you made me look. Much thanks.

Yep, that was all it was. Thanks to everyone else who tried also. Now onto the next problem with it, because there probably is one.

[IanB]

I forgot I clear the bit to avoid adding multiple negative signs if there are more digits. After I made that change, I fried a display (bad soldering), and completely forgot about it. I could not remember I changed the flag in a function, until you made me look. Much thanks.

You should probably redesign that code to avoid future problems of the same nature. Functions should not have unexpected side effects, and you have just discovered the reason for that rule.

Instead of having a function signature like:

Code: [Select]

void writeToDisplay()
you could have something like this instead:

Code: [Select]

void writeToDisplay(int number, int flags)
By passing into the function (by value) the information needed, you don't need to access (and potentially modify) global data.

Also, since the number is signed, it seems the "flags" bit to indicate negative numbers might be redundant?

[admiralk]

Yes, it is sloppy code, I fully intend to clean it up, as I go along, which also means ditching all the arduino code. I am taking baby steps, and throwing a lot of mud on the walls.

Functions should not have unexpected side effects

I could not agree more. That is why I had so much trouble finding the problem.

Also, since the number is signed, it seems the "flags" bit to indicate negative numbers might be redundant?

The number is independent of what is displayed. If the current number is 3, and I push the -5 button, the display will read 2 without telling it to include the negative sign (the whole purpose of FLAGNEG). This project is ultimately going to be a calculator. The other flags bits will be used for things like decimal points and whatever else I might need. The individual displays only have a 74HC595 feeding them, so feeding them an integer, signed or unsigned, is pointless. This is about as simple as t gets.

[IanB]

The number is independent of what is displayed. If the current number is 3, and I push the -5 button, the display will read 2 without telling it to include the negative sign (the whole purpose of FLAGNEG). This project is ultimately going to be a calculator. The other flags bits will be used for things like decimal points and whatever else I might need. The individual displays only have a 74HC595 feeding them, so feeding them an integer, signed or unsigned, is pointless.

I have trouble following your logic. You would have a display register containing the number to be displayed, and a display update function that makes the contents of the display match the contents of the display register. You would call the display update function every time the display register changes value. The negative sign is always available from the contents of the display register, something like this:

Code: [Select]

bool negativeSign = (number < 0);
There is no need to maintain a separate flag for this purpose.

This is about as simple as t gets.

I don't know. You seem to be adding complexity by having too many conditional paths through the code. The simplest code will have as few "if" conditions and branches as possible.

[abraxa]

https://www.tutorialspoint.com/cprogramming/c_bit_fields.htm

Do Arduino sketches support C99? If that's the case and you insist on using flags within the numbers themselves, I'd suggest using bit fields: https://www.tutorialspoint.com/cprogramming/c_bit_fields.htm

That way, you could reserve e.g. 5 bits for the magnitude of the number and 3 bits for whatever else (assuming you want to use 8 bit altogether).

[Ian.M]

https://www.tutorialspoint.com/cprogramming/c_bit_fields.htm

Do Arduino sketches support C99? If that's the case and you insist on using flags within the numbers themselves, I'd suggest using bit fields: https://www.tutorialspoint.com/cprogramming/c_bit_fields.htm

That way, you could reserve e.g. 5 bits for the magnitude of the number and 3 bits for whatever else (assuming you want to use 8 bit altogether).

Arduino uses the GCC toolchain and apart from the .ino file munging is ISO C++11 compliant, which is nearly entirely backwards compatible with C90, so yes, bitfields are on the table for this task.  However C++11 doesn't support C99 designated initialisers for structs. so the O.P. cant define a struct type variable with its elements initialised by name, so if the flags need to be initialised to a non-zero state, then either its got to be done by name as statements in Arduino setup(), (or in loop() or whatever other function if its defined with automatic scope)  or they must be supplied as a comma seperated initialiser list in element order.

I do feel that taking a step back from the coding and formally specifying the required inputs and outputs + drawing up a system level flowchart would be helpful, and if one is deliberately avoiding using C floating point, then careful consideration of data formats will be required and a decision needs to be made whether to use fixed point, some sort of bignum library or whether one is in fact going to emulate a four bit calculator and roll ones's own limited precision, limited range BCD float format.   Jumping into the coding without a clear overall design structure leads to mistakes like the bug above due to modifying the flags:FLAGNEG bit in incompatible ways in different places in the code

[admiralk]

Ahh, sleep is a good thing.

Let me try to clarify a little. I used the flags register because I am sure that I will need to track more than just if the number is negative. Perhaps it is a bit of premature optimization, but easy enough to revert if I do not end up needing it. The function was not supposed to change it, that was my mistake. I am not used to using global variables, so was not thinking properly when I did that. I solved the problem by making a temporary copy of it, and using that. Is there a better way, most likely. I am just trying to get it working like I want it to at this point. My next step is to figure out why it is only displaying 1 digit in negatives. In other words, negatives only work up to -9, after that it goes to -0, -1, .... I half expected this problem because I, consciously, did not worry about handling negatives at the start.

I do feel that taking a step back from the coding and formally specifying the required inputs and outputs + drawing up a system level flowchart would be helpful...

That is basically what I am doing here. It might seem roundabout, but trying out code help me find the limitations I am facing.  I could easily do this for a PC, in fact I think my first project in school was a calculator in VB. I have never used fixed point math before, and only just started looking at it, but I think that is what I am going to use. I have about a month before I will have a keypad, and be able to experiment, so plenty of time see how that works, or if I want to go a different route. I am unsure of what other requirements I will have. My objective is more about learning to write micro-controller code than produce an actual product. Arduino is just an easy way get something running since I also have to learn the electronic aspects also. 

[Ian.M]

Yes, that was premature optimisation.   The *EASY* way to do an integer display routine is to write it to work only with positive integers, and on entry to it, if negative, negate it (to  make it positive), output the number, digit by digit, then prepend '-' if it was initially negative.   If you are using a serial output like a UART, you need  to buffer the result digits and '-' as if you are using the classic K&r itoa() algorithm they are generated in the reverse order to that which you need to send them.  There is no need for a global result_is_negative flag, because (assuming signed integers) that information is contained in the high bit of the result which is set for all negative numbers.

It gets tougher if you are doing binary fixed point maths, as many binary fractions don't map well to limited precision decimals, so at that point it may be worth considering using BCD maths routines and packed BCD number storage.

[admiralk]

that information is contained in the high bit of the result which is set for all negative numbers

Now I understand what you are saying. It never crossed my mind to think of the number, itself, in binary terms. I think I like that way of looking at it, thanks.

[Ian.M]

that information is contained in the high bit of the result which is set for all negative numbers

Now I understand what you are saying. It never crossed my mind to think of the number, itself, in binary terms. I think I like that way of looking at it, thanks.

If you consider all  numbers as both their value and bit pattern, a lot of operations can be vastly simplified although you have to be careful about portability issues due to varying integer sizes between platforms/compilers.

e.g. see https://graphics.stanford.edu/~seander/bithacks.html

[admiralk]

Thanks for that link. I am finding I need a refresher on a few other things as well, sizeof() for example. I cannot seem to remember anything these days if I do not use it for while.