-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Intermediate merge of Banana Pi updates #993
Conversation
@akuker Not that many comments from my side, BUT:
Mixing pointer operations with integer operations looks very wrong in general, but especially when you have 64 bit pointers but 32 bit integers. As soon as everything is fixed (compiling) I will review again and then I will also run tests on the Pi. The scsiloop tool is a good idea, by the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pleas see my comments, not that many.
Thanks for taking a look! I think I addressed your comments. |
@akuker I'm afraid there are more compile-time issues. The unit tests do not compile on Bullseye anymore (neither on the Raspberry nor on the Banana Pi):
I strongly recommend not to use the boost library. Not just because it is not installed by default (at least not on Bullseye), but also because a dependency on boost is introduced. This is similar to having a dependency on a particular compiler version. Depending on the OS version the installable boost version can vary, and code using boost may not compile.
If it is about temporary files, the existing unit tests already make use of them without boost, see test_shared.cpp. The convenience methods in test_shared.cpp might be just what you need.
What is the "linker wrap functionality"? Only the new tests are affected by this? One more issue: rascsi and rasdump on the Raspberry Pi (but not on the Banana Pi) now segfault when not being run as root. On the develop branch there is no crash.
When creating #1013 I wondered why the platform appears to be detected at runtime. I did not dig deeper into this, but that's how it is, isn't it? The platform can only be either a Raspberry or a Banana Pi, i.e. code that is not needed should not be compiled/executed. This will make the binaries leaner, reduces compilation times and avoids issues during runtime detection. The more platforms are supported (e.g. other Pi flavors) the more problems will arise otherwise and the binaries will grow because of unused code. #1013 is probably the most elegant solution, but even conditional compilation based on #ifdefs would help. On the Raspberry Pi I did not find any issues except the ones just mentioned. On the Banana Pi things were not looking so good. Booting from an image did not work at all. Launching the hard disk driver from floppy disk sometimes worked, but often the attached drives were not detected. Reading sectors sometimes worked, and sometimes failed, cause unknown. Sometimes the rascsi process was frozen and could not even be killed anymore and I had to reboot the Banana Pi. Sometimes I was able to launch an application, but this was very slow, which might have been caused by the excessive logging. When testing with the Banana Pi, except for the Pi board my hardware and software setup was exactly the same as with the Raspberry Pi. I guess you do it anyway, but in case you don't I suggest to test by connecting two RaSCSI boards. On one you can run rasdump, for instance, on the other rascsi. This will provide logs from both the initiator and target perspective. |
Agreed. I didn't realize that the std::filesystem functionality existed. I switched the code over to use this instead.
I moved my temp files to the test_shared.cpp files as well. I also updated the existing functions so that they put the process id in the file path.
Linker wrap functionality allows you to command the linker to call a "__wrap" function instead of the real function. I've used this in several projects in the past for testing features that call lower level routines. For now, I've used it to override fopen(). So, when the gpiobus_raspberry.cpp class calls fopen() to read the /proc file system, the __wrap() function will re-direct it to a test file. This is enabled using linker arguments about which function(s) you want to wrap. See
Fixed.
There is already a precedent where the rascsi service detects which Raspberry Pi its running on and adjusts its behavior accordingly. So, for now, I'd propose that we leave the detection in.
Thank you for testing it out. Your results align with what I was seeing. When the banana pi did work, it was incredibly slow. (but it rarely worked). Inquiry seemed to work semi-reliably. When trace logging was enabled, it would consistently lock up, so I'll need to reduce the amount of logging. Definitely more work to be done on the BPi side of things.
I hadn't tried that before tonight. This is an awesome quick test to make sure things are working. I've been thinking about setting this up as a Self-hosted runner and have a couple dedicated Pi's just to do that test. But.... that's a "someday" task. I think I have all of your concerns addressed. Thank you again for looking at this! |
@akuker Thank you for addressing my comments. My main concern is that the Banana Pi code is not working, at least it is very instable. What are going to be the next steps? |
Agreed. The BPi functionality is unusable. My intention is that this PR should cover the larger "architectural" changes to support BPi. I'm assuming that any future updates will be much more localized and easier to merge. By merging this in now, we can get started with the "re-branding" to PiSCSI in parallel to me finishing up the BPi functionality. This is still my highest priority task and I'm planning on getting it fixed asap. Since the RPi functionality still works the same, the changes should not negatively affect any users. I understand/agree that it hurts our quality metrics for more untested code, but I'll be working on that as well. |
@akuker I am going to approve the PR, because I understand your reasoning. Nevertheless, giving approval for code that does not work (even if it may not affect any existing functionlity) makes me feel uneasy. |
This is not directly related to this PR or RaSCSI, but I mention it here because it also deals with the Banana Pi, in a general way.
I don't know whether you ever tried, but I gave up trying to boot the Banana Pi with WLAN access only. There is hardly any information on it, and I had a hard time to get a working WLAN at all. Now its working (but I do not even know why) but the Pi only boots when it is connected to the LAN. Very inconvient when your computers are placed somewhere where only WLAN is available. |
This is an interim merge of the Banana Pi updates to make things a little easier later. Some notes on the current state of things:
TODOs (before PR):
Defer until next PR:
Defer to new issues to deal with later: