@uweseimet - I'd really like to get some preliminary feedback from you on the architectural changes that I've made to support multiple types of "GPIOBUS" and "SYSTIMER" classes.

I'm not happy that I had to change the global 'bus' from a C++ reference to a pointer. But, I didn't see an easy way to create a reference to GPIOBUS, since we don't know at initialization whether its a RaspberryPi GPIO or Allwinner GPIO. Do you have any suggestions on how to handle this differently?

Note: This hasn't been tested. I'm really just interested in your feedback on the higher level architectural changes.

I'll probably abandon this PR after I get things working.

@uweseimet - Also note: These changes break the unit tests. I'll need to update those as well. I just really wanted some preliminary feedback before I dig into it more.

@akuker All in all there are not that much changes, at least not compared to what I am used to ;-). As expected the impact of the changes is essentially limited to the files in the hal folder, so that the overall architecture does not change. I also don't see any conflicts, at least nothing worth mentioning, with the pending changes of my upcoming PRs for issues 772, 827, 909 and 911.

Using a smart pointer for the bus looks fine for me, and there is probably no alternative when there is more than one bus implementation. References have advantages, but they cannot be used everywhere.

I suggest to replace DWORD with uint32_t. I have already done this everywhere else. The explicit std:: namespace should be dropped, like everywhere else. This does not cause any ambiguities.

Is the custom systimer class really needed? The C API offers quite a lot of portable timing functions up to the nanoseconed level.,I wonder whether systimer offers something that the C API does not provide. Anything we do not implement ourselves does not need unit tests and does not cause bugs.

Regarding the unit tests I don't expect many changes. Almost all of them use a mock for the bus (the abstract BUS, not the gpiobus), so they should hardly be affected.

@uweseimet - I'm struggling to get the unit tests to run. Would you have a few minutes to take a look to see if there is something simple I'm missing? I'm guessing it has to do with the change from references to shared_ptr<>'s.

As an example, the app is crashing in this test while trying to initialize the MockScsiController.

TEST(ScsiControllerTest, GetMaxLuns)
{
	MockScsiController controller(0);

	EXPECT_EQ(32, controller.GetMaxLuns());
}

Its crashing in the shared_ptr code

I definitely need to do some more training on the GMock functionality in the near future :-(

Is the custom systimer class really needed? The C API offers quite a lot of portable timing functions up to the nanoseconed level.,I wonder whether systimer offers something that the C API does not provide. Anything we do not implement ourselves does not need unit tests and does not cause bugs.

Agreed that anything we can use from the OS is better for us. The OS calls give control back to the scheduler, so we are at the mercy of the scheduler when the task get resumes. The Systimer works as a busy loop so that the task can resume more quickly without handing control back over the OS.

I did a very very quick and dirty experiment where I switched out the systimer class for the nanosleep calls. RaSCSI wasn't recognized by the host computer when I did that change. Its something that we could explore in the future if we need. But for now, I'd like to just keep the SysTimer.

I suggest to replace DWORD with uint32_t. I have already done this everywhere else. The explicit std:: namespace should be dropped, like everywhere else. This does not cause any ambiguities.

Agreed - will work on this, along with the sonarcloud warnings next. (Once the unit tests run again)

The API timer methods might have a special behavior wrt signals, blocking, or when waiting for 0 seconds, which might make them imcompatible with what's needed for RaSCSI. But I also think that it might make sense to re-visit this question in the future. It might turn out that the API calls also have benefits for RaSCSI, besides getting rid of custom code.

@akuker I am going to have a look at the tests soon.

By the way, clang++ reports several issues like these (I expect SonarCloud to report the same), and it is justified that the compiler complains:

./hal/gpiobus.h:362:18: warning: 'GetSignal' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

Note that clang++ compiles much faster than gcc and usually has better error-reporting. This is why on my PC I switched to clang++. This gives much better turnaround times.

@akuker Regarding the unit test: The problem is that MockScsiController owns the same bus object twice: Once in the AbstractController base class, and once in the MockScsiController. Despite the fact that the bus is a shared_ptr this appears to result in double deletion of the bus object in the destructor of ScsiController. I am not familiar with all the technical details, but this wierd kind of object ownership in the mock controller does not really appear to be a good idea, does it?

The resolution is to remove the MockBus field from MockScsiController and to apply this simple formal modification to the tests using MockScsiController:

TEST(ScsiControllerTest, GetMaxLuns)
{
	MockScsiController controller(make_shared<MockBus>(), 0);

	EXPECT_EQ(32, controller.GetMaxLuns());
}

Now there is definitely no potential competition on who (the base class or the derived class) destroys the bus, and with this minor change the test is passing:

[ RUN      ] ScsiControllerTest.GetMaxLuns
[       OK ] ScsiControllerTest.GetMaxLuns (0 ms)

If you agree I can just update all affected tests as part of the upcoming PR for issue 911, so that we get rid of this issue right away. Sooner or later somebody would have stumbled upon this anyway.

@akuker Regarding the unit test: The problem is that MockScsiController owns the same bus object twice: Once in the AbstractController base class, and once in the MockScsiController. Despite the fact that the bus is a shared_ptr this appears to result in double deletion of the bus object in the destructor of ScsiController. I am not familiar with all the technical details, but this wierd kind of object ownership in the mock controller does not really appear to be a good idea, does it?

The resolution is to remove the MockBus field from MockScsiController and to apply this simple formal modification to the tests using MockScsiController:

TEST(ScsiControllerTest, GetMaxLuns)
{
	MockScsiController controller(make_shared<MockBus>(), 0);

	EXPECT_EQ(32, controller.GetMaxLuns());
}

Now there is definitely no potential competition on who (the base class or the derived class) destroys the bus, and with this minor change the test is passing:

[ RUN      ] ScsiControllerTest.GetMaxLuns
[       OK ] ScsiControllerTest.GetMaxLuns (0 ms)

If you agree I can just update all affected tests as part of the upcoming PR for issue 911, so that we get rid of this issue right away. Sooner or later somebody would have stumbled upon this anyway.

Thank you for finding that! I'll try to make the change in my branch. Sonarcloud doesn't report any results if the tests don't run, so I want to get this addressed in my branch ASAP.

Thanks @uweseimet

@uweseimet - FYI: I also had to make the same change to MockAbstractController (at least with the changes in this branch)

@akuker That's not unexpected (I just had a look at the GetMaxLun test), because both mocks set up the bus in the same way.

@uweseimet - this one is ready to look at. Could you review it when you have a chance? there are a few sonarcloud CodeSmells that I'll update later. But, I'd like to get this merged into develop ASAP. I feel like I've spent a non-trivial part of my day trying to keep this branch up with develop.

@akuker I trust that you are going to fix the remaining issues soon.

Looks as if there are quite a lot of Banana Pi flavors?

I am going to approve even though your branch does not compile on clang++, because clang++ is not the reference compiler:

/usr/bin/x86_64-pc-linux-gnu-ld: /usr/bin/x86_64-pc-linux-gnu-ld: DWARF error: invalid or unhandled FORM value: 0x23
./obj/fullspec/rascsi.o: in function `InitBus()':
rascsi.cpp:(.text+0x30e): undefined reference to `ScsiController::LUN_MAX'
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:192: bin/fullspec/rascsi] Error 1

As already mentioned, clang++ is more convenient to use for development than gcc, because it compiles much faster.

You might want to clean up the commit message before merging (like I usually do because often some commit comments are not useful to be part of the commti history), this is why I have not merged yet.
I would appreciate if the compiler issue issue was fixed before merging.

@akuker I trust that you are going to fix the remaining issues soon.

Definitely!

Looks as if there are quite a lot of Banana Pi flavors?

Ya, for now I'm targeting the AllWinner H3-based version of Banana Pi M2 Plus

I am going to approve even though your branch does not compile on clang++, because clang++ is not the reference compiler:

I didn't see that exact message, but I did have others when I built with clang++. With the most recent changes, the tests built and ran on Ubuntu 22 with the following command:
make test CXX=clang++ -j8

Can you check if you're seeing the same compiler error with clang after these changes?

Note: I don't think Make is properly handing the header dependencies when using clang. So, you may need to manually clean after you change a header file.

Note that I did add a clang formatter configuration file. If you want to run clang-formatter on your changes, feel free to run:
clang-format -style=file whatever.cpp

style=file tells clang format to look for a .clang-format file in the current or one of the parent directories. Let me know if you want to make updates to this .clang-format configuration file. Its just a starting point, but it seems close-ish

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
29 Code Smells

8.7% Coverage
12.4% Duplication

@akuker I just noticed that on trace level when stopping rascsi numerous messages "I'm a Pi 4" are logged. Looks as if some debug code has not yet been removed?