Framebuffer interface

[bkirwi]

This rearranges the public API slightly to be more explicit: the user can now call Framebuffer::new explicitly, or ::classic or ::rm2fb to pick a specific backend.

Internally, this switches to using an enum for the old/new backend. This should be about as efficient to the old option-based interface, but moving the file descriptor used for ioctls into the enum makes certain types of errors less likely. My actual motive was to try and add a noop backend to allow for easier testing, but I'll leave that for a followup.

Haven't tested this properly yet, so marking as draft. Interested in thoughts if anyone has them!

[LinusCDE]

Cool! Wanted to something similar but it is still very much a draft and I'll probably do another approach since forcing anyone to deal with a Framebuffer as Trait Object is probably cumbersome to just making a similar thing in the backend (link).

Only glanced over the changes so far but look promising! One thing I would change is to use some other name than classic as this refers to historic development that new devs would only get confused by. Some suggestions: native, device or ioctl as you already used (though may not be clear for everyone). My personal favourite is the first one.

Also while at it, we might just change from_path(path) back to new() or go with impl Default/default(). While changing new(path) to from_path(path) was the correct move a while back, since nobody will ever need to specify the path anyway, placing the burden on devs to figure out what path is correct when (even if Device provides it) is just a wast of their time. We can keep the from_path to make a intentional decision easier, but then the path is kinda a magic value which we use to determine the backend. So not really clean anyway and could have undefined/unexpected behaviour (ub) if a dev chose to, for some reason, have another path for the framebuffer.

[LinusCDE]

Similar to classic, we could also offer rm2fb to have the crucial arguments specified. Those are both the path to shared memory and the sysv msg queue id (msqid). Just a minor nitpick though as both are currently constants. Not sure if they will ever change anyway.

[LoganDark]

How would native work on rM2? Would it just do nothing since rm2fb is seemingly the only program able to draw there?

[bkirwi]

[...] forcing anyone to deal with a Framebuffer as Trait Object is probably cumbersome to just making a similar thing in the backend (link).

Ah, didn't realize you'd started on this! I agree the trait object seems more cumbersome, and so far it looks like the performance should be ~equivalent.

One thing I would change is to use some other name than classic as this refers to historic development that new devs would only get confused by. Some suggestions: native, device or ioctl as you already used (though may not be clear for everyone). My personal favourite is the first one.

Agree that it's not a good name. It's awkward that the "native" interface is only "native" for RM1... but anyways probably the best thing to do is write good docs and encourage users to pick the default/autodetect if they don't know they need something else.

Also while at it, we might just change from_path(path) back to new() or go with impl Default/default().

Yeah, I think the from_path interface no longer makes sense for the design. Right now it lives deprecated alongside a new new() method, but I could delete it entirely right away... we're not worrying too much about backcompat in the 0.X series AFAICT. Better to get rid of it now, or leave the stub?

[bkirwi]

Similar to classic, we could also offer rm2fb to have the crucial arguments specified.

I'm very happy to do this for classic or whatever we rename it to, but rm2fb hardcodes some stuff fairly deep internally, so I'd prefer to take that one as followup... if that sounds okay?

[bkirwi]

How would native work on rM2? Would it just do nothing since rm2fb is seemingly the only program able to draw there?

That has the same behaviour as the currently-published release, which is: work perfectly if the program is run with the rm2fb client shim (which translates the syscalls to the rm2fb eqivalent instead) and not at all otherwise.

[LinusCDE]

Agree that it's not a good name. It's awkward that the "native" interface is only "native" for RM1... but anyways probably the best thing to do is write good docs and encourage users to pick the default/autodetect if they don't know they need something else.

Agree that this can cause confusion. Though of it as "native to linux". But people may think of it more like "native to remarkable". ioctl is probably better here. I think device would still be technically correct (linux device node), but people would most likely confuse this as well.

I'm very happy to do this for classic or whatever we rename it to, but rm2fb hardcodes some stuff fairly deep internally, so I'd prefer to take that one as followup... if that sounds okay?

Sounds okay!

[bkirwi]

I think I'm satisfied with this now, if anyone wants to take a look... I've renamed the method and added a bunch of docs.

I've done some testing as well on my RM2 -- all combinations of the methods and the client shim work as expected. (ie. everything displays to the screen except the device method when the client shim is not in use, which is normal and correct.)

[LinusCDE]

Will try to review this tomorrow! 👍

[fenollp]

We should move this outside the unsafe block as it's not an unsafe expression per-se also it'd collapse the ioctl call into fewer lines. (shaving that yak)

[fenollp]

LGTM!
The examples should be updated to not use the deprecated API but that can also happen in a later PR.

[bkirwi]

Great! Merged to avoid any further drift.

For followup, I have that rm2fb path param and cleaning up the deprecated usages. I'll likely get to that by early next week.