Output higher-level report from scsimon

[akuker]

Previously, scsimon would generate a VCD file that was viewable in GTKWave. This was fine for low level debugging, but wasn't very helpful when debugging higher level functions.

scsimon will now generate a .html report that shows the data being sent across the bus, mostly agnostic of the signal timing

scsimon can also be now run offline using a json data dump. This is mostly useful during development of the HTML file. But, it could be useful in the future to re-process a data dump.

[uweseimet]

• Instead of the old fprintf syntax more modern C++ output functionality should be used. Also, I'd suggest not using a single printf line per html statement but C++ raw string literals. This improves the readability a lot and makes formatting much easier.
• Removing -Wno-psabi may cause issues (bogus warnings) with some compiler versions. This is why -Wno-psabi was added in the past.

[uweseimet]

I just tried raw string literals with rasctl, see feature branch "feature_raw_string_literals". Formatting strings is simplified, most current programming languages support this concept.

[akuker]

• Removing -Wno-psabi may cause issues (bogus warnings) with some compiler versions. This is why -Wno-psabi was added in the past.

I removed it because it was causing issue with macOS Monterey. I intended to re-add it, but I missed that. Sorry!

I re-added it, but for Linux only. Any objections with this change?

[uweseimet]

@akuker Fine for me. I only tried with Linux anyway, and with Linux I noticed that you get warnings when omitting this option. The warnings tell you that the object code cannot be linked with object code generated by old gcc compilers. But since we do not use these old compilers the warnings are not relevant fir RaSCSI but just confusing.

[akuker]

• Instead of the old fprintf syntax more modern C++ output functionality should be used. Also, I'd suggest not using a single printf line per html statement but C++ raw string literals. This improves the readability a lot and makes formatting much easier.

@uweseimet - could you check out these changes?

Note: I didn't update the VCD file to use C++ style fstreams. I really didn't change that functionality, so I'd rather just leave it alone.

[uweseimet]

@akuker That's a pity, because the code would need a cleanup. The coding style is even more outdated than the platforms using the RaSCSI software ;-).

[akuker]

@akuker That's a pity, because the code would need a cleanup. The coding style is even more outdated than the platforms using the RaSCSI software ;-).

@uweseimet - You made me feel guilty, so I updated the VCD code as well. Could you take a look when you have a chance?

[uweseimet]

@akuker Actually, my main concern is code like we have it in sm_html_report.cpp, which as far as I can see is a typical scenario where raw string literals would be used. Would you mind if I changed sm_html_report.cpp so that string literals are used, at least in order to see how the resulting code would look like? Being able to compare how the code looks like before and after the change might make a final decision easier.
By the way, using streams avoids problems with 32/64 bit datatypes. Something like "printf %d" with an int argument will work on a 32 bit platform, but on 64 bit you usually get an error message because "%ld" would be required for the int type. I had lots of these issues when adding 64 bit support to RaSCSI. With C++ streams there is no such issue because the right format is automatically used by the compiler.

[akuker]

@akuker Actually, my main concern is code like we have it in sm_html_report.cpp, which as far as I can see is a typical scenario where raw string literals would be used. Would you mind if I changed sm_html_report.cpp so that string literals are used, at least in order to see how the resulting code would look like? Being able to compare how the code looks like before and after the change might make a final decision easier.

Absolutely. I would appreciate any guidance you can provide.

[uweseimet]

Please see the scsimon_json_string_literals branch for how the first two methods of sm_html_report.cpp might look like when using string literals. The resulting output should be identical. Variables (the filename in this case) are not literals, this is why the last lines in print_html_header are unchanged.

static void print_html_header(FILE *html_fp, const char *html_filename){
	string html = R"(
<html>
<head>
<style>
table, th, td {
  border: 1px solid black;
  border-collapse: collapse;
}

.collapsible {
  background-color: #777;
  color: white;
  cursor: pointer;
  width: 100%%;
  border: none;
  text-align: left;
  outline: none;
}

.active, .collapsible:hover {
  background-color: #555;
}

.content {
  padding: 0;
  display: none;
  overflow: hidden;
  background-color: #f1f1f1;
}

</style>
</head>
)";

    fprintf(html_fp, html.c_str());

    fprintf(html_fp, "<h1>%s</h1>\n", html_filename);
    fprintf(html_fp, "<table>\n");
}

static void print_html_footer(FILE *html_fp){
	string html = R"(
</table>

<script>
var coll = document.getElementsByClassName(\"collapsible\");
var i;

for (i = 0; i < coll.length; i++) {
  coll[i].addEventListener(\"click\", function() {
    this.classList.toggle(\"active\");
    var content = this.nextElementSibling;
    if (content.style.display === \"block\") {
      content.style.display = \"none\";
    } else {
      content.style.display = \"block\";
    }
  });
}
</script>

</html>
)";
    fprintf(html_fp, html.c_str());
}

All in all with string literals much less typing is required, and one can easily read the styles and the JavaScript. Essentially you can just copy existing style/js code as is into the C++ source file and that's it.

Besided using string literals in cases like this, using C++ streams instead of C file handles usually needs even less code and avoids issues with platforms where the length of 32 and 64 bit data types and their format strings differ, as already mentioned.
With a stream instead of a FILE you can just write the literals to the stream without a temporariy string:

stream << R"(
</table>

<script>
var coll = document.getElementsByClassName(\"collapsible\");
var i;

for (i = 0; i < coll.length; i++) {
  coll[i].addEventListener(\"click\", function() {
    this.classList.toggle(\"active\");
    var content = this.nextElementSibling;
    if (content.style.display === \"block\") {
      content.style.display = \"none\";
    } else {
      content.style.display = \"block\";
    }
  });
}
</script>

</html>
)";

No files are required anywhere.

[uweseimet]

By the way, something may be wrong with the Makefile dependencies. After modifying sm_html_report.cpp the compiler does not automatically rebuild.

[akuker]

By the way, something may be wrong with the Makefile dependencies. After modifying sm_html_report.cpp the compiler does not automatically rebuild.

I just noticed that I had two copies of the same files checked in. I'm guessing you were looking at the one in the 'scsimon' folder. I have deleted that folder with old copies of the files. (That's why the Makefile wasn't catching them).

Could you take a look at the ones in the 'monitor' folder? I think those are what you were going for :]

[uweseimet]

@akuker Since I have already modified one file in my temporary in order to show an example for the benefits of string literals (see my detailed comment above) there is no need to work on other files. The difference should be obvious, based on comment above and the existing modified file. I just noticed the problem with 'make' by accident.

[akuker]

@akuker Since I have already modified one file in my temporary in order to show an example for the benefits of string literals (see my detailed comment above) there is no need to work on other files. The difference should be obvious, based on comment above and the existing modified file. I just noticed the problem with 'make' by accident.

Hi @uweseimet - I'm in agreement with you on string literals, and I believe I have made updates to use them. I believe there was just an extraneous copy of the file in the repo. I have deleted these extraneous files.

Could you take a look at these three files in particular? I believe these achieve what you were requesting. There

• https://github.com/akuker/RASCSI/blob/feature_scsimon_json/src/raspberrypi/monitor/sm_vcd_report.cpp
• https://github.com/akuker/RASCSI/blob/feature_scsimon_json/src/raspberrypi/monitor/sm_json_report.cpp
• https://github.com/akuker/RASCSI/blob/feature_scsimon_json/src/raspberrypi/monitor/sm_vcd_report.cpp

If these look OK, could you approve this pull request?

[uweseimet]

@akuker The code below might also qualify, even though it cannot be just a single string literal because of DATE and TIME in between. I guess whether to convert this or not is a matter of taste. Literals are best, of course, if there is just plain pre-formatted text without anything else in between.
I am going to approve your PR (thank you for applying the changes), and leave it up to you whether or not to also convert the block below.

static void print_copyright_info(ofstream& html_fp)
{
    html_fp << "<table>" << endl \
            << "<h1>RaSCSI scsimon Capture Tool</h1>" << endl \
            << "<pre>Version " << rascsi_get_version_string() \
            << __DATE__ << " " << __TIME__ << endl \
            << "Copyright (C) 2016-2020 GIMONS" << endl \
            << "Copyright (C) 2020-2021 Contributors to the RaSCSI project" << endl \
            << "</pre></table>" << endl \
            << "<br>" << endl;
}