shermp / go-fbink

Go wrapper for fbink
GNU Affero General Public License v3.0
2 stars 1 forks source link

Any potential issues? #1

Open shermp opened 6 years ago

shermp commented 6 years ago

So I may have created a Go wrapper for the FBInk program.... oops?

@NiLuJe do you have any potential concerns or feedback with this project before I "announce" its existence to the few people who might care?

I'm still trying to decide whether to include the source of FBInk directly, include it as a submodule, or just leave a link to the source repository (current action).

NiLuJe commented 6 years ago

I'm not really familiar with Go, but if either the C header is needed, or you introduce a minimal build system, I feel like a submodule makes sense, if only because you're then a git pull away from updates ;).

NiLuJe commented 6 years ago

And definitely no concerns, I went with C and a library both because that made sense for KFMon, but also with FFI in the back of my mind... ;)

shermp commented 6 years ago

Yeah, the C header is required (for the same reason it's required in C).

Thanks for the feedback. I don't know if anybody else will use this little project, but hey, it will be here if anyone needs it.

NiLuJe commented 6 years ago

Speaking of, if anything feels clunky or weird API-wise, I'm open to any kind of feedback, this was a first for me, and I don't really have any kind of CS background, so this was pretty much an experiment that went mostly right, somehow ^^.

shermp commented 6 years ago

Seems fine to me. I'm not exactly a CS guy myself, and I've been more of a part time hobby dev who gets a bit of inspiration every few months. I've probably done more (serious) development in the past couple of months than the previous several years.

The API documentation in the header is very clear, concise, and it worked first time.

The reason for trying out Go with kobo-rclone was I had just come off adding VHD support to an emulator (in C), and I couldn't face the thought of having to deal with strings in C...

shermp commented 6 years ago

FBInk has been added as a submodule and installation instructions have been modified accordingly. Thanks for the feedback.

NiLuJe commented 6 years ago

See this bit of git trickery to make the submodule init recursive by default, which might feel slightly less arcane ;).

NiLuJe commented 6 years ago

Random nitpick regarding the README:

In the second example,

fbinkOpts.Row = 8
gofbink.FBinkInit(-1, fbinkOpts)
gofbink.FBinkPrint(-1, "This is another test", fbinkOpts)

that (second) Init call is (usually*) superfluous, the config fields that affects init's behavior are those listed here. Row/Col not being one of them ;).

(That's assuming the three examples are taken as one, and not plucked out and isolated, because you do need at least one init before any print* ^^).


[*] That's leaving aside the possibility that the Kobo switched fb modes in between, which should never happen during the "normal" uptime of a Kobo, once Nickel has started.

(On 32bpp FW, very early boot and pickel (i.e., mainly the the boot progress bar) are the only thing that could cause a switch to 16bpp).

shermp commented 6 years ago

Thanks for the submodule trick. I shall also modify the readme.

shermp commented 6 years ago

Actually, I just had a thought regarding the API. The function signatures are a bit confusing, because they do not name the parameters, only type.

This makes it harder to use without having the header open as a reference.

NiLuJe commented 6 years ago

Yeah, I was thinking about that, too. I can't quite remember when/where I picked that up. And I'm not sure I like it much either ;D.

On the upside, that means you kind of have to write documentation, because otherwise it's a game of "hey, guess what this does"! On the downside, well, there's no variable name to help you figure out what it might do without reading the doc if you're in a hurry.

You also don't have to bother keeping signature & implementation in sync in terms of variable names, because, well, one side doesn't have them ;). But that should pretty much be a non-issue with any decent IDE.

TL;DR: Yeah, agreed, I'll fix that ;).