Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MarkCountAsUsed() function is not working properly #10

Open
JoLo33 opened this issue Aug 25, 2022 · 2 comments
Open

MarkCountAsUsed() function is not working properly #10

JoLo33 opened this issue Aug 25, 2022 · 2 comments
Labels
bug Something isn't working

Comments

@JoLo33
Copy link

JoLo33 commented Aug 25, 2022

Hey,

it is possible to enter an older Add token than an entered Set token in the C implementation. I figured out that the MarkCountAsUsed() function is not working properly.

The macro CHECK_BIT(variable, position) is not good.

#define CHECK_BIT(variable, position) (((variable) >> (position)) & 1)
 int test = 0;    
test = CHECK_BIT(0b111, 2);

test will be 4. That's ok if you use the check only in if() but it will be used also in the SET_BIT(variable,position,value) as value and the value should only be 1 or 0. That leads to

#define SET_BIT(variable,position,value) (variable & ~(1<<position)) | (value<<position)
#define CHECK_BIT(variable, position) (((variable) >> (position)) & 1)
int test = 0;    
test = SET_BIT(0b0,0,CHECK_BIT(0b111, 2));

and test will be 0b100 and not 0b1 as expected.

Also the for loop is not working correct.
The function will still not work correctly with the new CHECK_BIT macro from above.

There are two problems:
1.

uint16_t maxcount = 0;
	uint16_t UsedCounts = 0;

	MarkCountAsUsed(4,	&maxcount, &UsedCounts, 1);
	MarkCountAsUsed(8,	&maxcount, &UsedCounts, 1);
	MarkCountAsUsed(16,	&maxcount, &UsedCounts, 1);

After this, UsedCounts is 0b10000000, but that is wrong because counts 4 and 8 are now marked as unused again.

MarkCountAsUsed(33,	&maxcount, &UsedCounts, 1);
MarkCountAsUsed(34,	&maxcount, &UsedCounts, 1);

After the first example, the counts 33 and 34 will should be marked. Count 33 will set UsedCounts to 0b1111111111111111, that's correct, but count 34 will set it to 0b111111111111101 so that count 32 is marked as unused, which is not correct.

If the for loop will be changed to following, it will work

for(int i=RelativeCount+1; i < MAX_UNUSED_OLDER_TOKENS - RelativeCount; i++) {
                NewUsedCount = SET_BIT(NewUsedCount, i, CHECK_BIT(*UsedCounts, i-RelativeCount));
            }

Regards Jonas

@JoLo33 JoLo33 changed the title CHECK_BIT macro in opaygo_decoder is not good MarkCountAsUsed() function is not working properly Aug 26, 2022
JoLo33 added a commit to JoLo33/OpenPAYGO-HW that referenced this issue Aug 29, 2022
changed: CHECK_BIT macro, return only 0 or 1
changed: for loop in MarkCountAsUsed, so that the correct counts are marked
changed: IsCountValid, so that the limits are correct as explained in general documentation
@benmod
Copy link
Member

benmod commented Oct 20, 2022

Hi! Thanks for reporting the issue, can you provide a bit of details on the type of MCU and compiler you are using? This might be an endianness issue.

@JoLo33
Copy link
Author

JoLo33 commented Oct 20, 2022

Hey, of course:

MCU: STM32F401RETx
IDE: CubeIDE V: 1.5.1
Toolchain Version: Type: GBU Tools for STM32 / Version: 7-2018-q2-update
STM32CubeMX: 6.1.1-RC2
Firmware Package: STM32Cube FW_F4 V1.25.2
FreeRTOS: 10.0.1
CMSIS-RTOS: 1.02
Optimization level: -Og

@dmohns dmohns added the bug Something isn't working label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants