search

PiSCSI / piscsi

PiSCSI allows a Raspberry Pi to function as emulated SCSI devices (hard disk, CD-ROM, and others) for vintage SCSI-based computers and devices. This is a fork of the RaSCSI project by GIMONS.
https://piscsi.org
BSD 3-Clause "New" or "Revised" License
545 stars 82 forks source link

C++ code reviewers needed #1242

Closed uweseimet closed 1 year ago

uweseimet commented 1 year ago

A lack of reviewers is once again slowing down the development and maintenance of the PiSCSI C++ core code: Tickets ready for a PR and existing PRs cannot be merged within a reasonable timeframe. Other tickets cannot be worked upon because they require pending PRs to have been merged before. Help is appreciated by anybody who is familiar with OOP and modern C++, and in the ideal case also with SCSI.

dialtr commented 1 year ago

I have significant C++ experience, but no real experience with SCSI. Would be willing to review code; please let me know what you're looking for in terms of help!

uweseimet commented 1 year ago

@dialtr That's really cool!

From my perspective, the main problem we have had with the C++ code in the past is that the original rascsi legacy code was more C than C++ and suffered from countless problems. This is not just my subjective view but was confirmed by a huge number of issues reported by SonarQube. Since then, besides adding a lot of new features (https://www.piscsi.de/en/piscsi.html provides a list with the most important), substantial work on the code structure etc. was required in order to address these problems. Reviewing these changes is a lot of work and in the past @akuker was the only one who did the reviews, while I was almost the only code contributor working on getting the C++ code into a better shape. In the meantime I have re-implemented large parts of the C++ core, resolved more than 1500 SonarQube issues and added quite a lot of unit tests. cpp/devices/cfilesystem.cpp still shows how the old code was looking like, and just this file causes most of the remaining SonarQube issues.

Anyway, there are development phases with a lot of resolved C++ tickets (related to new features, structural changes or to improvements like C++-20 support) where it would be great to have more than one potential reviewer. This is why I ask for help in this ticket. IMO skills in modern C++ and OOP are more important than a deep insight into SCSI. Most of the code (except for the /hal folder) is not that low-level, but it is more about how controllers/devices/command executors/threads interact and how they are structured. The goal is C++ code that is up to date, portable, robust and expressive. It should make use of best practices of contemporary C++, both from the language and the design perspective. Just because this projects deals with old computers does not mean that the C++ code should look like C code from the 80s or 90s ;-).

I just created an new PR, by the way, https://github.com/PiSCSI/piscsi/pull/1247. I did not manage to add you to the reviewer list, though. https://github.com/PiSCSI/piscsi/issues/1214 has the details. This is one of the typical tickets trying to address code problems like wrong responsibilities or minor bugs that become apparent while the code is evolving.

dialtr commented 1 year ago

Uwe, thanks for the note.

I definitely have experience with old, anachronistic codebases and have been involved in modernizing them in the past. Where I may also face challenges is that my own C++ knowledge is a bit stale. I've been working towards learning the modern features lately, but in my role as a manager, it's not part of my day-to-day to code much anymore. At the same time, I think that if some of the problems are deeper than just C++20 migration, I could be of help. I'm going to start by taking a look at the code and any docs that exist in the repo, as well as the PR.

uweseimet commented 1 year ago

Sounds good, thank you!

Before getting involved in this project I had not used C++ for more than a decade, and the state of the rascsi codebase 3 years ago was a good incentive to update my outdated C++ knowledge. Since then I have read numerous books on modern C++ and watched quite a lot of conference talks (videos) on current C++ and the STL. All this has hopefully had some benefits ;-). Regarding modernizing codebases I needed to do that quite a lot for C++ and Java.

Wrt the C++ features we may use we are restricted to a subset of C++-20, because the versions of both g++ and clang++ supported by Debian/Raspberry Pi OS are usually quite old. What I would have liked to have is views and not just ranges, and maybe support for "using enum", but so far I could live with the compiler limitations. We should ensure that the code compiles everywhere, also on a regular Linux PC, and runs on bullseye and bookworm, on Raspberry Pi OS and Ubuntu, 32 and 64 bit. As far as possible all tools should also run on regular Linux PCs, because this is very helpful for testing everything not related to the PiSCSI board hardware. And, of course, for doing the actual development work. Even a Pi 5 would be too slow for that. Most of the code deals with things where you do not need special hardware for anyway.