Skip to content

Improve how commands are internally executed#1247

Merged
uweseimet merged 7 commits intodevelopfrom
issue_1214
Oct 30, 2023
Merged

Improve how commands are internally executed#1247
uweseimet merged 7 commits intodevelopfrom
issue_1214

Conversation

@uweseimet
Copy link
Copy Markdown
Contributor

@uweseimet uweseimet commented Oct 15, 2023

  • Use const CommandContext for execution
  • Update error handling
  • Fix SonarQube issues
  • Remove duplicate code
  • Use mutex instead of atomic_bool

* Use const CommandContext for execution
* Update error handling
* Fix SonarQube issues
* Remove duplicate code
* Use mutex instead of atomic_bool
* Add hasher
* Add param_map
@uweseimet uweseimet marked this pull request as ready for review October 15, 2023 08:13
@uweseimet uweseimet linked an issue Oct 15, 2023 that may be closed by this pull request
3 tasks
@uweseimet
Copy link
Copy Markdown
Contributor Author

@akuker Less changes this time ;-).

I have not managed to add @dialtr to the list of reviewers. Any idea why? I thought anybody could be added.

context.SetDefaultFolder(piscsi_image.GetDefaultFolder());
return ExecuteCommand(context);
}, port); !error.empty()) {
cerr << "Error: " << error << endl;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere I see reference to a logging function (spdlog::info, etc). Is this write to cerr leftover from debugging and should it be converted to a proper log message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, everything that is logged to a logfile (usually by the PiSCSI core) is logged with the spdlog library. Messages the result from the user launching command line tools (e.g. scsictl, piscsi) are usually written to cout or cerr, in order to provide direct feedback to the user (e.g. because a command line parameter was wrong). There is no need for those to be logged with timestamps etc.

In the old C++ code you may still find macros use for logging, but they should gradually be replaced by direct usage of spdlog. The macros have issues, e.g. there might be a buffer overflow. Current C++ tries to reduce the use of macros anyway because of several reasons.

Which IDE or editor are you using, by the way? And which kind of Pi? My main development environment is Eclipse on Linux, a Pi 4, and mainly Atari computers for testing that requires SCSI functionality.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Yes, down with macros!

Typically, I use vim as an editor, but have also been using CLion lately. I note that you have a clang-format configuration file defining the formatting style, and CLion plays nicely with that, because it can enforce formatting rules. Thus, I am likely to use CLion more for this project.

In "production", my PiSCSI is connected to a Raspberry Pi 3 Model A+, but I also have a 4 lying around. I am using this as a drive emulator for my Akai S3000XL and Akai S6000 samplers, and will also be able to test it on a Kurzweil K2000R sampler at some point.

While the Tindie store is currently out of stock, I am just going to invest in another PiSCSI when they are available so that I can maintain a development system separate from the one that I use for music stuff.

I've forked the repo and have been poking around. Are there build instructions anywhere for Debian amd64? Or is the procedure to run that script?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.

If you want to compile anywhere, e.g. on a regular PC, there are notes how to do this at the bottom of https://github.com/PiSCSI/piscsi/wiki/Setup-Instructions.
I guess running easysetup might work (@rdmark probably knows more), but when I set up a new compile environment I usually just check out the sources, install the libraries/headers that may be required for compiling (there is a list of them in the easysetup script), and then just run make.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the formatting style file was added by @akuker. I have never used it, and I do not know how closely it reflects the current style.

@uweseimet How about we review the coding guidelines and agree upon a shared set of stylistic rules? With a new developer onboarding, I think it would be productive to have this to avoid f.e. unexpected line changes when someone's IDE automatically changes indentation or line breaks, etc.

I think this is the file in question: https://github.com/PiSCSI/piscsi/blob/main/cpp/.clang-format

Let me create a new ticket for this.

Copy link
Copy Markdown
Contributor Author

@uweseimet uweseimet Oct 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdmark I agree. But I would like to merge all pending PRs and the tickets that are already ready for PRs but already may need merging (there are several) first. Otherwise formatting changes will make the existing changed code unusable. Does this make sense?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uweseimet Yes I agree with that priority. I will create the ticket so that we can take action on this a little bit later.

Comment on lines +88 to +89
// TODO Check why there are rare cases where bus is NULL on a remote interface shutdown
// even though it is never set to NULL anywhere
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a bug report for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I don't think it is needed. The null check is a working solution, but not one I am satisfied with. All in all this is just something that should be improved, but nothing that affects the user. This is why I think a TODO is sufficient.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will merge after resolving the merge conflicts.

@uweseimet uweseimet merged commit 8bd06ea into develop Oct 30, 2023
@uweseimet uweseimet deleted the issue_1214 branch October 30, 2023 12:57
@sonarqubecloud
Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

20.3% 20.3% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve how Piscsi and PiscsiExecutor execute commands

3 participants