Add built-in swtfb client and enable musl builds on rM 2

[LinusCDE]

See #76 . This should also enable musl builds and hence solve that problem.

Code should be semi clean. As mentioned in #76 we might want to change "Core" and "Swtfb" into seperate backends (also todo when doing breaking changes: use AsRef<std::path::Path> in from_path).

Testing if wanted:

• Musleabihf build should work for demo example
• Demo example with libc should run with AND without shim (e.g. LD_PRELOAD=... or rm2fb-shim).

Artifacts built with cross build --target=armv7-unknown-linux-gnueabihf --release --example demo and cross build --target=armv7-unknown-linux-musleabihf --release --example demo : demos.tar.gz

New:

• framebuffer::swtfb_client
• examples/swftb_sysv_spy.rs as dylib (for LD_PRELOAD)
• device::Device::get_framebuffer_path(&self) -> &'static str: Recommended to use in never versions as arg for Framebuffer::from_path(). Otherwise new swtfb_client will not be used automatically.

Breaking changes:

• get_fix_screeninfo, get_var_screeninfo, put_var_screeninfo have got an additional parameter to work with swtfb_client. I doubt that anyone used such low level functions apart from me in plato.

Other changes:

• Removed some duplicate code in FramebufferRefresh
• Added new field swtfb_client: Option<super::swtfb_client::SwtfbClient> to Framebuffer

Note: With swtfb_client, FixScreeninfo->smem_start will not be set (= 0) since it's hard to do without splitting the from_path completely for both variants. Again I highly doubt that anyone used this. Framebuffer.frame.as_ptr() should return the correct value.

[bkirwi]

This is very cool!

I'm not familiar enough with the SysV stuff for a proper review, but I read through / left a few questions mostly for my own education. Feel free to ignore them if I'm missing the point. :)

I'll give this a spin on my own RM2, both with the standard demo app and a couple others, and report back. Hopefully within the next couple days.

[LinusCDE]

This is very cool!

I'm not familiar enough with the SysV stuff for a proper review, but I read through / left a few questions mostly for my own education. Feel free to ignore them if I'm missing the point. :)

I'll give this a spin on my own RM2, both with the standard demo app and a couple others, and report back. Hopefully within the next couple days.

Thanks for the review! I totally missed those parts. Nothing anything of this was related to how SysV is supposed to work. Mostly C and Rust tidbids, but important oversights and bad practice I didn't even know about. Huge thanks!

[LinusCDE]

Made send_xochitl_update non public for now. Seems to not be meant for normal clients. I personally would still like the full structure for completness and e.g. swtfb_sysv_spy.rs.

Any idea how to proceed best?

https://discordapp.com/channels/385916768696139794/386181213699702786/909142226381176842

[fenollp]

Great work! I should be able to test this on both archs soon, I'm excited.
I think you're right about using thiserror and also about refactoring to implement swfb as traits.

Not sure why there was discussion on dynamic dispatch for traits though? but sure do whatever

[fenollp]

No idea if it's needed. How does one can produce the nested case?

// from ddvk's ipc.cpp : nesting only triggers logging a line
  uint16_t *mem =
      (uint16_t *)mmap(NULL, BUF_SIZE, PROT_WRITE, MAP_SHARED, fd, 0);

  if (getenv("RM2FB_NESTED") == "") {
    fprintf(stderr, "OPENED SHARED MEM: /dev/shm%s at %x, errno: %i\n",
            name.c_str(), mem, errno);
  }
  return mem;

[LinusCDE]

Not sure either. Seems like a debugging case but the behaviour isn't strictly altered when a nested app is detected. The spec/design also doesn't mention anything here.

[LinusCDE]

Is handling this important in the client @raisjn ? Seems to be merely used for more accurate debug output.

[raisjn]

it's not necessary, mostly was about debugging some stuff. not even sure nesting works correctly. i think rmkit uses RM2FB env variable to detect if it is on rm2, but don't think libremarkable needs it

[LinusCDE]

Okay. Removed this with commit 6a8dbe1 for now then. If this is ever needed for advanced checking of errors or such, we can add it again.

[fenollp]

Shouldn't this be in a loop?
I have no idea if the clock here can jump around forward/backwards in large/small leaps...

[LinusCDE]

I hadn't bothered redoing this with rust types yet. Thought of this, too but rm2fb did the same and got away for it. For the sake of not needing to test this myself, the code is basically a copy for now.

I also had done some things like that for work. There are weird cases where such stuff is meant for leap seconds and rust's chrono and/or time crates handle conversion from timespec a lot more strictly (even though this just seems like a tame overflow counter). So didn't really wanna tamper with the already working solution. But your point seems pretty important as I know think about it more.

@raisjn any insights here? May be an issue for your client as well. Maybe this never mattered since the timeout never exceeded it in testing?

[LinusCDE]

For now I think we should just keep it as close to the original client as possible. If the client changes this, we can do so as well later. But it may be a bug that just got never triggered.

[raisjn]

I don't remember the reasoning here or why we did this, but a loop sounds right. there might be something in the git log for rm2fb about it (use blame?)

[LinusCDE]

The commit refers to a pr, where the actual commits where squashed (already before merging).

The source commit is this one: ddvk/remarkable2-framebuffer@495c051

I guess it worked here, since the max timeout is 200ms and so one wrap around is enough. It would probably be correct to use while instead of if in case anyone would increase the timeout >1s.

We should probably fix this on both sides for correctness.

[LinusCDE]

The performance could probably be even faster/branchless when using something like this instead:

timeout.tv_nsec += SEM_WAIT_TIMEOUT;
+ timeout.tv_sec += timeout.tv_nsec / 1e9;
+ timeout.tv_nsec %= 1e9;
- if (timeout.tv_nsec >= 1e9) {
-   timeout.tv_nsec -= 1e9;
-   timeout.tv_sec++;
- }

[LinusCDE]

Added the above change with commit 3bd976c.

To get this done here I opened a PR here for the original client to address it separately there: ddvk/remarkable2-framebuffer#81

[fenollp]

WRT send_xochitl_update: if it helps with reverse engineering I'm fine with it.
It's out of the spec so let's just document that and it's all good :)

[LinusCDE]

WRT send_xochitl_update: if it helps with reverse engineering I'm fine with it. It's out of the spec so let's just document that and it's all good :)

Agreed. I'll probably still like to keep the function private (or just remove it). You can just construct a swtfb_update easily yourself and send with the public send() method anyway. So the lib wouldn't actually prevent you from doing this, just require a bit more knowledge which seems like a fine trade-off.

[LinusCDE]

I think all standing points should be resolved now.

[bkirwi]

Nice work!

I'm personally comfortable with merging... any API cleanup / etc. we decide we want can happen in future PRs.