Code applies wrong value in certain situations

[XTC1977]

I have a code section that was updated to write to a single bit in a file a flag of 1 or 0. A 1 = speed is in a 5ph increment, i.e. 55mph. A 0 = speed was divisible by 10.

The reason for dividing by 10 is the file has just 2 bits to store the speed and has to support speeds greater than 100.

The code i have is setting the 5ph flag on most records but is failing in certain situations.

A 0 speed value is a valid value and seems to be part of the cause here

Faults:
- First record if a 0 speed is set to 5ph flag = 1 incorrectly.
- If a 5ph record is encountered and the flag set to 1, i.e. 55mph, and the next record is a 0 speed then the second record is also having the 5ph flag set to 1 when it should be 0. this stays the same until a new speed that is a simple divisible by 10 speed is encountered.

To me it is like once a 5ph speed has been encountered and the flag set to 1 then until another valid speed /10 is encountered, all subsequent records that follow, if 0ph speeds are set to flag = 1 incorrectly. Kind of like the default flag setting has switched from 0 to 1.

Any help or knowledge offered would be greatly appreciated as this is the only thing wrong in this process.

Here is the section of code specifically:

sCamera.ucSpeed = sSpeedLimit / 10;
if(sSpeedLimit)
{
sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
}

Here is the code at the end of the section with some greater context in case it helps:

bool CDataExport::SetupCameras()
{
CComPtr<ICameraIVUIter> pICameraIVUIter; // Handle to CameraIter
CComPtr<ICameraIVU> pICameraIVU; // Handle to Camera
CComBSTR bsClause = "";
CComBSTR bsOrderBy = "ID";
CameraData sCamera;
BYTE *ucCameras = m_pIVUDB->ucCameras;
long lCameraID = 0;
long lFirstWaypoint = 0;
short sSpeedLimit = 0;
short sMake = 0;
CComBSTR bsType = "";
CComBSTR bsStatus = "";

// Initialise
memset(&sCamera, 0xff, sizeof(CameraData));

try
{
// Setup Iterator
SAFE_CALL(m_pICameraIVUs->get_Iter(bsClause, bsOrderBy, (IDispatch**)&pICameraIVUIter));

// Get First
SAFE_CALL(pICameraIVUIter->get_First((IDispatch**)&pICameraIVU));

// Process Loop
while(pICameraIVU != NULL)
{

// Get & Write Camera Data
SAFE_CALL(pICameraIVU->get_ID(&lCameraID));
SAFE_CALL(pICameraIVU->get_FirstWaypointID(&lFirstWaypoint));
SAFE_CALL(pICameraIVU->get_SpeedLimit(&sSpeedLimit));
SAFE_CALL(pICameraIVU->get_Type(&bsType));
SAFE_CALL(pICameraIVU->get_Status(&bsStatus));
SAFE_CALL(pICameraIVU->get_Make(&sMake));

// Allocate Structure
if(lFirstWaypoint > 0)
sCamera.dwWaypointIndex = m_pIndex[lFirstWaypoint].dwHash;
sCamera.ucSpeed = sSpeedLimit / 10;
if(sSpeedLimit)
{
sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
}

[salem c]

I would suggest you prototype the logic in a separate program.

#include <iostream>
bool is5mph(int speed) {
  return speed % 5 == 0;
}

int main ( ) {
  for ( int speed = 0 ; speed < 20 ; speed++ ) {
    std::cout << speed << "," << is5mph(speed) << endl;
  }
}

When you've got your little function doing the right thing, you can just drop it into your main code.

[XTC1977]

Thanks Salem.

That is an interesting sample and approach.

This program is quite old and sensitive to change but this code mostly does what it should except it seems to get the flag stuck and not parse each limit individually.

Could you suggest a mod to this particular code as i am not a C programmer but know a solution was applied in minutes by someone else for which they never delivered the source for it.

The first part is fixed and works fine for normal speed limits:

sCamera.ucSpeed = sSpeedLimit / 10;

The updated part below apparently just needs a change to make it work more on a per record basis but i just don't know C. Can this just be done in a change to this IF statemtn:

if(sSpeedLimit)
{
sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
}

Thanks again and to anyone else with a solution here.

[ne555]

> sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
the mod operation is irrelevant there, you are only checking if the number is odd or even
that would work if `sSpeedLimit' may only end in 0 or 5

if(sSpeedLimit)
{
   sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
}

per your description the issue is when `sSpeedLimit' is 0, in which case it does not enter the body of the `if' and .uc5ph remains uninitialised or keeps its old value.
I don't see the point in the condition, just drop it

/*if(sSpeedLimit)
{
   sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
}*/
sCamera.uc5ph = ((sSpeedLimit % 10) == 5);

[XTC1977]

Thank you for this solution. The speed indeed will always end in 0 or 5 but then a 0 is also valid and as you say is not being handled and appears also to stick with th old value as per your explanation.

I have passed this onto my team to review but hopefully moves us to the final solution.

[dhayden]

A 0 speed value is a valid value and seems to be part of the cause here

Perhaps because of this:

// Initialise
memset(&sCamera, 0xff, sizeof(CameraData));
...
if(sSpeedLimit)
{
    sCamera.uc5ph = ((sSpeedLimit % 10) & 1);
} else {
    // sSpeedLimit == 0 and sCamera.uc5ph remains 0xff
}