ams-hackers / gbforth

👾 A Forth-based Game Boy development kit
https://gbforth.org
MIT License
121 stars 23 forks source link

Add sha256 example #342

Closed lf94 closed 1 month ago

lf94 commented 2 months ago

Hey hey!

Huge new Forth lover here. I did this as an exercise in learning and exploring Forth.

(Edit: this is now fixed :)

For some reason, on Game Boy, the hash is not the same as on PC (gforth). I have wrote the program in a way to account for the Game Boy's CPU characteristics, but I must have missed something. I would love if I could get some help correcting the issue.

Here's a little accompanying video: https://www.youtube.com/watch?v=QGpZl23vXbw

The hash output should be F62EC8A06B1F3B271C6A2A64EA367FF439E138561EA580C9866D910000000000.

Once this issue is corrected, it'd be nice to then merge. I don't feel great requesting a merge of something technically bugged.

lf94 commented 2 months ago

To be clear, I'm looking for any insight on some assumption I've made about Forth on Game Boy. Clearly there's a system-specific word that's affecting the final outcome.

lf94 commented 2 months ago

Tomorrow I'll begin to debug this.

lf94 commented 2 months ago

Still working on it, but this PR will also introduce [if] [then] :)

lf94 commented 2 months ago

It has been corrected!

tkers commented 1 month ago

Hey! Sorry for noticing this before, though looks like you found the problem already :) I'm assuming you ran into some overflow/wrapping issues, or what went wrong initially?

Just tried to run the ROM and I think the hex visualisation is a little off still: 6FE28C0AB6F1B372C1A6A246AE63F74F931E8365E15A089C68D6190000000000 I think removing the extra swap from hex-byte-to-chars would probably do the trick.

lf94 commented 1 month ago

The issue was memory layout, yeah. Going back to a byte-based system fixed everything. Good eye on that output :) I'll fix that up - then we good to merge? :D

davazp commented 1 month ago

👏 wow, that is amazing!

lf94 commented 1 month ago

gbforth is amazing :) I love that this system will most likely work until the death of the Universe. As someone who has a soft spot for the DMG + CGB series and now Forth, I'll probably come back to this excellent project time and time again. You guys have really done something special here.

lf94 commented 1 month ago

Oh yea, the video was updated (the old one removed). https://www.youtube.com/watch?v=QGpZl23vXbw I've updated the top comment too. The video goes into "why forth" a bit :)

lf94 commented 1 month ago

Also, I know I'm not from Amsterdam but I'd love to help maintain this project. It seems even just taking a peek once in awhile to engage with issues and the odd merge would be greatly appreciated.

tkers commented 1 month ago

If the hash visualisation works it's good to merge, yes!

One thing I realised regarding your overflow issues is that gbforth uses a CELL size of 2 whereas in desktop/gforth it's typically 8. Not sure if you managed to trace it to that or just went with the byte-based approach directly, but I'm guessing that could be the reason for the different outputs.

Some thoughts on your code, in the interest of learning:

(to clarify: that's not a request to make changes, just sharing some ideas)

lf94 commented 1 month ago

Love the thoughts, thank you!

Not sure if you managed to trace it to that or just went with the byte-based approach directly, but I'm guessing that could be the reason for the different outputs.

I actually started with a byte-based approach, then went to a 16-bit cell size approach, then got stung hard by endianness and memory layout issues between gforth and gbforth. It was just too much headache to keep track of. I went back to the original byte based approach and everything worked flawlessly again, and much cleaner.

For cases like CREATE foo 1 cells allot you can also use VARIABLE foo. More of a stylistic choice.

Good observation; I indeed choose CREATE over VARIABLE as a stylistic choice. I'm not a fan of the abstraction VARIABLE does over memory allocation.

Great to also have an example of portable code! Surprised we didn't have [IF] yet, it's very useful.

My [IF] seems to be not very robust by the way. I wouldn't mind if after one of you look into it, or if it's "good enough". For example, you will not be able to nest them.

there are standard words to deal with double-cell

I have learned you cannot really trust the standard is available on all forths. Implementing the few words I needed here is good for portabiilty.

If you care about the performance on-device

Not particularly in this case.

Use :m

I avoided this because I thought it was not very portable to use. It is a very nice feature though!


While developing this I've been in ##forth on irc.libera.chat so I've had some input along the way which closely matches what you've said :)

I will try to submit the fixes tonight. I recently received my STM32s to try out mecrisp-stellaris on, so I may be busy with this. I just need to remember to put aside ~10 minutes to do the fix :). Alternatively feel free to merge and commit the fix in master.

lf94 commented 1 month ago

@tkers all fixed, you were right about the swap :)

tkers commented 1 month ago

Awesome!

My [IF] seems to be not very robust by the way. I wouldn't mind if after one of you look into it, or if it's "good enough". For example, you will not be able to nest them.

I think it’s good enough for now since it’s already proven useful in your example. I’ll create an issue for nesting.