Oh I see @uweseimet is in the midst of a cleanup which will probably require some changes to sync up with that when his done. I'll park this here for now and update accordingly when the time is right.

@mynameistroy Yes, it would be better to wait. Can you please ensure that you PR does not duplicate code? On first sight it looks as if code is duplicate from SCSI_HD to SCSI_CD.
If you do not know how to avoid duplication I can help with this when the time for merging this PR has come. I guess that some code will end up in the scsi_command_util.

@mynameistroy I just had a closer look at scsi_command_util.cpp and think that there would not be many conflicts with my changes. We would probably be fine to merge your changes before mine, after any code duplication issues have been resolved.

@uweseimet sure I can clean that up easily enough.

@mynameistroy Sounds good!

Note: This is related to issue #1

Apologies @uweseimet, but the scsi_command_util has no reference to ModeSense pages just ModeSelect. So it's not entirely clear where something used by two devices (SCCD, SCHD) would go without the duplication you already mentioned as being undesirable. The mode_page_device seems like a likely placement but it apparently would likely be similarly overreaching like Disk seems to be.

I'm certainly happy to make whatever changes that work for you. I am sorry if I'm not getting what your desired implementation is.

@mynameistroy scsi_command_util does not deal with MODE SENSE yet;, but scsi_command_util is where this shared code belongs to. This why scsi_command_utill.h says "Shared code for SCSI command implementations". This is why I suggested scsi_command_util yesterday.
Whether it is mode sense or mode select or a different command does not matter. If there were already unit tests related to mode sense for all device types, changing the mode sense implementation in Disk would break them.

Alrighty, restarted this with a clean slate for clarity :)

I'm guessing the SonarCloud analysis failed because the PR is coming from an outside repo which does not have the SonarCloud 'secret' key configured. I believe this should resolve itself when this is merged into develop.

If not, we'll work through it.... I'm still learning about this tool

I've approved, but I'll wait for @uweseimet to approve the change before we merge.

Sounds good to me

Looks as you approved after all without waiting. I don't think this is the proper procedure, but anyway, on first sight the changes look fine to me now. Thank you, @mynameistroy .
#1 can be closed I assume?

@mynameistroy Please remember to confirm that #1 can be closed, because your change was merged.

@uweseimet Oh sorry, I thought you were still speaking to @akuker . Indeed you can close #1 now and again sorry for the back and forth.

@mynameistroy Cool! I will close it.