suloku / gcmm

A gamecube/wii memory card manager
GNU General Public License v3.0
255 stars 24 forks source link

card.c #40

Closed DacoTaco closed 3 years ago

DacoTaco commented 3 years ago

Yo,

i was just scrolling through the code and i noticed you have a copy of card.c in the repo. i was wondering why it's still there.

i diff'd it against libogc's card.c and i only saw you re-enabling 1 function (and exposing it), having a potential compare bug in your CARD_CreateEntryAsync (or in libogc?), a shit ton of newline replacements & 2 functions added. aka nothing too special.

is there a reason this wasn't upstreamed to libogc? need help upstreaming it so everyone can use the changes?

diff : card.txt

i also know you need the raw access for raw backup/restores, but it might be better to have global wrapper functions for those to be honest.

suloku commented 3 years ago

Well, libogc's card.c was "recently" updated to have propper memory card unlocking, before I was using tueidj's unlocking functions wich differed vastly and was easier to just have my own copy of card.c.

I just recently updated it to libogc's. The compare bug hasn't still been adressed in main repository (https://github.com/devkitPro/libogc/issues/113)

Honestly, I don't have a clue about how to use libogc's card.c and access the functions I need (programming is just a hobby for me, never had any kind of propper programming course or anything in any language).

I guess sadly my only response can be "it works".

DacoTaco commented 3 years ago

that explains some stuff hehe.

i came by the code by coincidence, and i don't like seeing stuff like that as it could make others think it's a good idea to do the same, instead of upstreaming changes for everyone to use :) (ive done this kind of stuff before as well, also for the card patches, but moved away from them once it was upstreamed in libogc)

I'll see if i can make the required changes in libogc to have the differences ( name compare bug fixed, the added functions + functions for the stuff you need ) so you shouldn't need it anymore! :) (unless of course you come across issues, then you can re-add it to debug hehe)

suloku commented 3 years ago

That would be awesome. Also, over the years there were other libogc bugs (already fixed) that were easier to debug with my own copy of card.c, but I don't think a new one is likely to appear after so many years, but you never know.

DacoTaco commented 3 years ago

ye, i had the same thing with ctr-gcs dt edition (but that code needs to burn in hell haha). using a copy of the library source can help to debug or use temp untill the library is fixed. also did that in priiloader after fixing something in libogc's ES code.

so far ive added the functions & fixed the memcmp bug in a local branch. i'll have to look into the __card_read/write/erase functions next. did i see it correctly that a card_sync is called after the write/erase functions? cause i was thinking of integrating it into the card_write & card_erase functions

DacoTaco commented 3 years ago

i created a new PR @ libogc ( https://github.com/devkitPro/libogc/pull/116 ) with the changes. i also have a GCMM fork that i used to make the changes (https://github.com/DacoTaco/gcmm/tree/CardFixes). once the changes are merged into libogc i will take the card.c/h from there and upstream my changes back into your repo.

this way, when a newer libogc version is released with these changes, you can just remove the card.c/h without any more problems

DacoTaco commented 3 years ago

Update : Wintermute merged my PR. My GCMM fork works with it, but apparently has issues when wanting to compile it with the older version of libogc. so ill leave my PR as draft until either libogc update is released or i find a (temp) work around

carstene1ns commented 3 years ago

With 2.3.0 released, we could just #ifdef the local copy out and use some #defines to use the renamed functions.

DacoTaco commented 3 years ago

ye, that was the idea. didn't have much time yesterday to do the actual fix.

see #41 , it looked like it worked on my end. could you verify @suloku ?

once a new libogc is released you can remove the card.c/card.h & remove the OLD_LIBOGC stuff

carstene1ns commented 3 years ago

Instead of using an extra OLD_LIBOGC define you can include the version header and use the actual version for comparison. This is cleaner.

DacoTaco commented 3 years ago

@carstene1ns : there is a define with the libogc version? i didn't know that lol i know libogc has some version defines in its makefile, but i didn't know those number were exported/defined somewhere else? (i mean libogc version, not devkitppc version or something)

carstene1ns commented 3 years ago

It is written by the Makefile.

So putting something like this in card.h should work:

#include <ogc/libversion.h>

#if (_V_MAJOR_ <= 2) && (_V_MINOR_ <= 2)

// old libogc, use this card.h/c
#define OLD_LIBOGC

#endif
DacoTaco commented 3 years ago

well shit, i didn't know that file existed. no wonder i didn't see when searching libogc, if its generated by the makefile.

ye, i'll be changing it to that (obviously)

DacoTaco commented 3 years ago

@suloku : a new libogc version, which has the fixes, was released today. want me to change the code in my pull request?

suloku commented 3 years ago

Hello you all, sorry for no responses, the time I can use for the scene is sadly minimal. I did see the pull request and have been meaning to test it for months... About defining libogc version, it would be a really good idea to use the libogc version definition if we are relying on libogc for the functions. I think using the local copy only makes sense if any modification/bug has to be resolved in card.c (as was the case until now). Old versions should be deprecated, and it's to be expected that older versions might not be fomoatible with newer library versions. I think just adding a "to compile version 1.5 and previous you'll need libogc 2.2.0 or lower, check release dates" or something like that...

In any case I have a little time this week, I want to look into this issue #27 , see if I can fix it and then release a new version with latest libogc.

DacoTaco commented 3 years ago

Hey, no problem. not everyone can sometimes no-life scene dev like i can :P

as is the pull request uses the version defines to either use the (now released libogc) or the included files using the #define lines.

just see when you have time to test! ^^

carstene1ns commented 3 years ago

Since older versions are not advised to be used anyway, I think removing the old stuff now makes sense.

suloku commented 3 years ago

After some testing, it seems problems with the 2048 block cards seem to arise from the fact that allocating 16MB of memory at once isn't a good idea (it did work...that many years ago I coded a simple backup/restore app and then coy pasted the code onto gcmm...). 1.5 seems to struggle with reading, probably due to newer libraries taking more space (?). 1.4g can still correctly dump a 2048 card, but 1.5 can't (at least on my end). I'm gonna rework the raw read/write functions so not as much memory needs to be allocated. I haven't tested, but maybe gamecube even struggles with 1024 block cards. Seems a good idea even if that's not the root of the problem.

DacoTaco commented 3 years ago

its possible memory allocation changes from libraries could screw that up if you allocated 1 16MB buffer in memory. MEM1 is only ~24MB, so maybe some other stuff are getting in the way? imo it might be better to do it sector by sector too and you may or may not kill #27 with it too :P

that said, i'm closing my issue report for now. unless problems arise , i think we can consider this done ^^