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

Web interface issues #1392

Closed uweseimet closed 11 months ago

uweseimet commented 1 year ago

I just had my first look ever at the (new) web UI. Looks good!

I will use this ticket to collect issues I stumble upon. This is in progress, so no need to react on any change ;-). I am using the current develop branch.

rdmark commented 1 year ago

thanks for testing the Web UI! It’s valuable to get fresh eyes on it.

Regarding point 2 — this is by design. The list at the bottom was originally conceived for device types that can be attached without specifying an image file.

To attach SCHD you pick an image from the image management list, instead.

But now when you point it out, it might be more intuitive to have SCHD in the bottom list as well. There is logic in the Python code that filters it out. Removing the filtering will get it listed. We could then add a drop-down like for the removable disk types, and just make choosing an image mandatory (I.e. default to the first image in the list.)

regarding point 3 — the Web UI actually creates the list of device types and parameters dynamically after querying the backend for its capabilities. SAHD should be supported automatically if a capability is added.

uweseimet commented 1 year ago

Regarding 3.: What does capability mean, i.e. capability of the backend or the UI? When the backend reports that the SASI device is supported the UI does not display it, but scsictl does (without a code change).

rdmark commented 1 year ago

By capability I mean of the backend.

When you say that a SASI device is not displayed, do you mean in the bottom table of peripheral devices?

adding + DISK_DEVICE_TYPES to this expression should get both SCHD and SAHD to show up in the table.

https://github.com/PiSCSI/piscsi/blob/796869dde6d7fac7143079de29e95ec9ad70e693/python/web/src/templates/index.html#L597C7-L597C7

Additional refactoring and special care handling: f e the aforementioned images drop-down, file ending mapping, localized UI strings etc, will be needed for a good UI however.

Once the SASI device type has been readded to piscsi it should be a quick job to update the Web UI.

uweseimet commented 1 year ago

I would have expected any device type that is reported by the backend to be offered in the UI. In this case localization may be an issue, though. In my app if there is an unknown type I just display something like "Unknown". This is sufficient for making use of such a type, because the parameter handling is the same for all types. Of course, this kind of flexibility is more important for a client app not coupled that tight to the distribution as the web UI. But also the UI may be connected to something else with the "--backend" option. Regarding this option there is one minor issue. I just mention it for completeness sake: When you use it and there is no images folder on the machine where the UI is running, you first have to create an empty image folder in order to use the UI. Just displaying a warning in the UI instead of requiring to create an empty local folder would be nicer.

rdmark commented 12 months ago

Yes the Web UI will also list unknown devices in a similar fashion (using the four letter key instead of a localized string).

The difference here is that I have a filter in place to not list fixed disk type device types (meaning it has the "supports_file" flag but not the "removable" flag) in that "peripherals" table, for the design reason that I explained earlier. I will put up a PR to remove that filter over the weekend to demonstrate this!

uweseimet commented 12 months ago

OK, I will try to have a look at this PR.

Note that there is no four letter key in the protobuf data for devices that are really unknown and not just marked deprecated. If there was a new SACD device, for instance, and you build your application against a .proto file that does not contain this enum, you would only see the ordinal value, but not the key. You might decide to just ignore such a device, of course. On the other hand, from the interface meta data you know a lot about it, (which parameters it supports etc. This means that you can offer an almost complete UI for such an unknown device. Only the key is missing. From the protobuf perspective there are no compatibility issues. protobuf is documented to support certain kinds of interface changes, as long as you do not change the data types of existing definitions.

rdmark commented 12 months ago

@uweseimet Here is the PR if you want to try it https://github.com/PiSCSI/piscsi/pull/1393

It should address each of your three pieces of feedback.

I'm particularly curious if it handles the SAHD device type gracefully now.

uweseimet commented 12 months ago

@rdmark I will definitely try, most likely later today.