cvra / platform-abstraction

Platform abstraction layer for microcontrollers
3 stars 6 forks source link

Use CPPUmock for critical sections. #31

Closed antoinealb closed 10 years ago

antoinealb commented 10 years ago

This allows for easier testing, since we can put in a test that we expect critical sections macro to be called (see the new tests for example).

This adds the possibility to test for critical sections in other functions, example :

TEST(SomeTestGroup, SomeTest)
{
    criticalsection_use_capturing_mock();
    mock().expectOneCall("CPU_CRITICAL_ENTER");
    mock().expectOneCall("CPU_CRITICAL_EXIT");

    foo(); // foo should contain atomic code.

    mock().checkExpectations();
    mock.clear();
}
pierluca commented 10 years ago

Doesn't LGTM. 1) You've given up on testing for proper allocation. Not good. 2) I can provide you with a buggy (previous) version of my code that will pass your test. There's a reason for measuring the critical section depth in both use cases. It's one of the possibly weak points of the macro implementation

antoinealb commented 10 years ago

1) You've given up on testing for proper allocation. Not good.

What do you mean ? I still test that CRITICAL_SECTION_ALLOC is called before CRITICAL_SECTION_ENTER.

There's a reason for measuring the critical section depth in both use cases.

Could you explain ? It is what I do in this test, at least I think :

TEST(CriticalSectionMockTestGroup, NestedWorksToo)
{
    CRITICAL_SECTION_ALLOC();
    mock().expectOneCall("CPU_CRITICAL_ENTER");
    mock().expectOneCall("CPU_CRITICAL_ENTER");
    mock().expectOneCall("CPU_CRITICAL_EXIT");
    mock().expectOneCall("CPU_CRITICAL_EXIT");

    CRITICAL_SECTION() {
        CRITICAL_SECTION() {

        }
    }
    mock().checkExpectations();
}
antoinealb commented 10 years ago

By the way I am just trying to understand. I will add the older tests back with those ones tonight.

pierluca commented 10 years ago

Sorry, clarirfication.

1) is independent of THESE tests. If in some other library, you're using CRITICAL_SECTION without having done CRITICAL_SECTION_ALLOC(), the previous implementation would fail because __mock_critical_depth is not declared. I believe this to be useful because it's a compile-time check. This can be added back quite easily by combining your modification with the previous code.

2) I should check my old version again and the CppUTest documentation, I might have been partially wrong with this one, sorry. The thing is that when nesting critical sections, the macro for-loop was doing weird things since the variables were shared and, if I remember correctly, the innermost exit was exiting all the critical sections.

For completeness this could be modified as

TEST(CriticalSectionMockTestGroup, NestedWorksToo)
{
    CRITICAL_SECTION_ALLOC();
    mock().expectOneCall("CPU_CRITICAL_ENTER");
    mock().expectOneCall("MOCK_ONE");
    mock().expectOneCall("CPU_CRITICAL_ENTER");
    mock().expectOneCall("MOCK_TWO");
    mock().expectOneCall("CPU_CRITICAL_EXIT");
    mock().expectOneCall("MOCK_THREE");
    mock().expectOneCall("CPU_CRITICAL_EXIT");

    CRITICAL_SECTION() {
        mockOne();
        CRITICAL_SECTION() {
                  mockTwo();
        }
       mockThree();
    }
    mock().checkExpectations();
}

What do you think?

antoinealb commented 10 years ago

cppumock doesn't enforce expectation order. But it would still check that everything is executed. Will change the test to this.

pierluca commented 10 years ago

What about "expectOneCall(FUNC).withCallOrder(N)" ? Expectation order in this case seems important.

antoinealb commented 10 years ago

I don't think call order is really important there (see my added test).

For 1), it still fails if you remove ALLOC because __critical_alloc is not defined.

pierluca commented 10 years ago

Oh yes right, I should have definitely taken more time to look at it :-/ my bad, sorry, busy week-end...

antoinealb commented 10 years ago

The build errors for now, probably due to a change in CppUTest. Need to fix the travis.yml before it can be merged.

antoinealb commented 10 years ago

The error was hotfixed, apparently there are some changes that break the build with CPPUTest, so for now I use a specific version of it.

antoinealb commented 10 years ago

@pierluca Did I adress all your concerns ? If yes, could you merge please ?

edit: In the meantime I enforced call order in critical section mock tests.

pierluca commented 10 years ago

I guess so, yes, thanks. Sorry, busy days, my latency is quite bad.