Verification Needed for 64-bit Flags Command on Multiple Platforms

I modified the Flags command to support 64bit. The display on WEBui is also fixed. Checked on the simulator and on BK7238. Is there anyone who can verify this on other platforms?

test:

flags 18446744073709551615




Added after 38 [seconds]:

PR: https://github.com/openshwprojects/OpenBK7231T_App/pull/1548

Wouldn't it break config on platforms where long is 8 bytes?

Btw, what was the original reason to make flags into 2 integers, instead of an uint64_t?

insmod wrote:

Btw, what was the original reason to make flags into 2 integers, instead of an uint64_t?

I don´t know. Maybe, first version have 32 flags only.

Added after 11 [minutes]:

insmod wrote:

Wouldn't it break config on platforms where long is 8 bytes?

You're right, changed to uint32.

Added after 3 [minutes]:

BTW, on which platform for OBK is a long actually 8 bytes long? Are they all 32 bit mcu's, or not?
It's my fault, I'm a developer mainly for Win and on 64bit Win (LLP64) long is 32bit

>>21456907 Currently all are 32bit, but who knows in the future. And while long on 32bit is usually 4 bytes, who knows what some compilers would do. I know that i've seen 4 bytes long on 64 bit windows.

>>21456953
OK, thanks to uint32, the flags are ready for the 64-bit future

I don't remember well, but I think I've started with a single 32-bit integer. Then I added another 32 bits it has turned out that there is a long-standing bug with flag set (or read?). It has been tacked several times but we didn't get any definitive fix, unless PR 1548 works well on all platforms?

Ok will test. Is the latest build looking final for testing?

Are there platforms I shouldn't bother with at all because it'll definitely not work? And should all the ESPs be treated as one test or one from each series required - like one ESP32-C#, one ESP32-S# etc

One thing I remember is that my initial flags code was working well on Windows. That's why the bug was not found for so long...

>>21457353
It is final. There is no need to test all devices, all of them are 32bit.
The thing to test is if funcion strtoull() works ok on the main SDKs (ESP, BL, LN). Beken is ok.

Hey, I've just got an idea!

Maybe we could do a selftest system like the one on Windows, but designed to run on Device?

For example, put it into the cmd_selfTest.c and make there a long list of commands with result assertions.

Then we could run it on physical devices to make sure that everything is okay.

This could be very useful, especially considering platform differences and such....

We already had issues related to those, for example:
- time_t size (32 bit vs 64 bit)
- missing realloc on BL602
- strange sprintf behaviour (remember static IP page?)

>>21457368

p.kaczmarek2 wrote:

Maybe we could do a selftest system like the one on Windows, but designed to run on Device?

I think this is not necessary. The main problems are revealed on win, and the "special" unexpected ones would be detected by the test with very little probability anyway.

Are you sure? I suspect that even at this moment realloc is still broken on BL602... @miegapele may know more about it.

p.kaczmarek2 wrote:

moment realloc

I don't know what the problem is with realloc, but if the same problem will be on windows, would selftest find it?

No, realloc implementation is per platform. That's why it's (as I suspect) broken on BL602, but works on other platforms.

Related issue:
https://github.com/openshwprojects/OpenBK7231T_App/pull/1529

That's why I think we may benefit from making a simple self-test that runs on any platform, with #define entry in obk_config.h , so it takes 0 space in Release builds.
It could test for basic stuff, like JSON parsing, commands execution, etc.

I mean, if we didn't know about it and the problem wasn't programmed to be checked

Added after 4 [minutes]:

p.kaczmarek2 wrote:

It could test for basic stuff, like JSON parsing, commands execution, etc.

but ok, the fact is that this would save a lot of work testing new things on platforms other than where they were debugged.

1548_merge_66c6fb20c815








All set flags without error apart from RTL-B which had a hard fault. It came back up with all but 1 set though





Added after 3 [minutes]:

setting flag 13 and saving thereafter does not cause a hard fault though

Added after 3 [minutes]:

interesting. with all flags cleared and only flag 13 set it'll hard fault if you move to the home page.

Added after 2 [minutes]:

I guess that's this it doesn't like


Added after 16 [minutes]:

but unrelated to the flags fix. The same happens with general release 1.18.47 and only flag 13

@divadiow Thanks for the great test. So just to be sure, the RTL error is not related to PR, or should I do something?

Added after 1 [minutes]:

divadiow wrote:

but unrelated to the flags fix. T

OK, I see.

I am adding self test system to build device.
https://github.com/openshwprojects/OpenBK7231...mmit/a00d88b2a112c260985661cd079b7a5deba9a014

Usage:
1. enable ENABLE_TEST_COMMANDS in obk_config.h
2. add your command to cmd_test.c


3. add it to drv_test.c:


4. do build

5. in build run:

100 means 100ms interval between steps.
It runs in such a way, that it first prints command it will run, it waits 100ms for log to come through, then runs command, then checks run result, then waits 100 ms, and pritns again....

Sample run:



The most important thing to note here is that if some command crashes, you can also run it from console without test driver.

Please give me initial feedback, but also wait 1 day before running it. I will improve it a bit soon.

p.kaczmarek2 wrote:

Please give me initial feedback

Yeah, that looks good. I'll definitely check it out later.

See this commit for test of math expression with setChannel:
https://github.com/openshwprojects/OpenBK7231...mmit/0d18dd0a229b6de6dfd1abd2a396c8de076e40c7
And this commit for some LLM-generated basic tests:
https://github.com/openshwprojects/OpenBK7231...mmit/1b455ef82ec026f914eccb7bea1f2a0ecbc5dcb5

Added after 1 [minutes]:

One of practical uses of this could be this cursed IP parse bug:

so, this is the right place for

and

test

Yes and this will be finally meaningful, and not pointless like testing on Windows, because those functions are outside our SDK and testing them on Windows won't tell us if they will work on Beken on BL602.

Added after 1 [hours] 18 [minutes]:

IP parse test:
https://github.com/openshwprojects/OpenBK7231...mmit/23e67076c3bf9b0b035ed2bb77066238277f76c9

this all sounds quite good. How exactly do self-tests work and help? I'm not very codey, but it seems like a sensible thing to have in place. If, for example, a contributor proposes code changes in a new PR and they cause a problem somewhere, how does the self-test manifest? What and where fails? Somewhere in the build workflow log?

I am thinking about creating a separate guide for that, but I can already start here.

So, there are two types of self tests.
1. Windows-only self tests - they are run in Windows simulator, they reside here:
https://github.com/openshwprojects/OpenBK7231T_App/tree/main/src/selftest
They are run on Github on each build:


Those self tests are basically simulating a use case scenario, for example, setting up a LED, and then they check externally is the use case result as expected.
For example, if we set two PWM pins, we expect CW control to show up and we expect Dimmer command to work. We expect certain things to be published when light state changse, we can test it as well. So, we simulate setting of two PWM pins, then we run some commands, and check is result as expected (for example, are output PWM values as expected for correct CW control).

Example 1 - if we set channel 1 to 123, is $CH1 constant working in script and expands to 123?

If "buffer" is not "123", then SELFTEST_ASSERT_STRING will show error on Github build (red cross instead of green checkmark).

Example 2 - if we set two PWM pins, are they correctly detected as CW light? Is light responding to commands and settings PWMs correctly?

Let's look deeper at one fragment:

This basically says:
- if you have CW light set up, and OBK receives a POWER OFF Tasmota command, the led_enableAll variable should be 0 (false) and both PWMs should be 0
- if you later receive POWER ON Tasmota command, led_enableAll is expected to be 1 (true) and for current configuration (set earlier in code) first PWM should be 100%, second 0% (because we earlier set up temperature 100% cold)

Thanks to this mechanism, as soon as somebody breaks the expected behaviour (for example, adds a change that makes CW lights not working), we will know at compile time, because self test will catch it.

2. Per-platform tests, introduced yesterday.
The test runner is here:
https://github.com/openshwprojects/OpenBK7231T_App/blob/main/src/driver/drv_test.c
The test commands are here:
https://github.com/openshwprojects/OpenBK7231T_App/blob/main/src/cmnds/cmd_test.c

Per-platform can be only run on true platform devices, like BK7231, not in simulator.
They were introduced because not everything can be tested in Windows simulator.
Some things are per-platform, for example sscanf command, or realloc, etc.
So we have "different" sscanf or sprintf on each platform - different on BK7231, W800, W600...

That's why I added test commands like this one:

This checks IP parsing, and this is required, because it has proven problematic, see implementation:

This is not just a theoretical sample - we really had this issue:

As you can see above, we had a sscanf problem that requires special handling on W600/LN882H/Realtek, and we didn't catch it early.
If we had per-platform device tests back then that cover str to ip, we would have caught it earlier.

It is impossible to catch this problem on WIndows self tests, because it is present only on some platforms with their specific implementation.

Thanks to the per-platform tests, it's now possible to catch it.
If you compile OBK with test commands enabled, and if your platform has str_to_ip broken, then this code:

will attempt to parse "192.168.0.123", but it will fail,, then "if" will detect it, so it will return CMD_RES_ERROR.
So, later, the drv_test.c will catch it, and it will show error here:


Self tests are very useful, because they can quickly check if all tested features can work as expected. You don't need to setup a CW light to check if PWMs are set correctly - this is done by Windows selftests in SImulator on each Github build. You also don't need now to manually check each page like local ip config on every platform, because per-platform tests will cover it as well....

How to run self tests?
- to run Windows simulator self tests, just trigger online github build, they are ran automatically
- to run per-platform tests, compile OBK with ENABLE_TEST_COMMANDS , flash it to your device, and run "backlog startDriver Test; StartTest 100;"

I didn't have time to run per-platform tests myself, but I strongly suspect that at least realloc test will just crash BL602 device - realloc is known to be a problem on BL602.

thank you. that is very interesting and makes things clearer. fascinating!

So now let's make a demonstration. We'll consider a hypothetical scenario where someone breaks some function by accident, for example, the channel set.
I modified CHANNEL_Set_Ex to always set value to 0:

Then I commited changes to Github.
Let's see what happens.
It's building, so we wait a moment...

And then we get:

AS you can see, many self tests have failed. They expect channels to work, so they recognized that something is wrong:

>>21458386
great work, as usual!

Added after 1 [minutes]:

so this build simulator test will automatically run on every PR/update?

Added after 55 [seconds]:

Or does it have to be started manually?

Build Simulator and the tests ran on Windows are nothing new, they are already in our Github for a month or more. They run with each commit.

The only new thing that I introduced yesterday is the per-platform testing that can be started only manually on physical device.

p.kaczmarek2 wrote:

The only new thing that I introduced yesterday is the per-platform testing that can be started only manually on physical device.

I see. It seemed strange to me how a test for a specific MCU HW could run on github

1548_merge_82ad074ae93b

no hard fault but flag 13 is deselected on reboot, otherwise seems OK on RTL8720D