Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C template #1981

Merged
merged 7 commits into from
Jul 20, 2022
Merged

Add C template #1981

merged 7 commits into from
Jul 20, 2022

Conversation

Gota7
Copy link
Contributor

@Gota7 Gota7 commented Jul 12, 2022

Implements a C WASM template and build system.

Implements a C WASM template and build system.
void map(int32_t x, int32_t y, int32_t w, int32_t h, int32_t sx, int32_t sy, uint32_t* transcolors, int32_t colorcount, int32_t scale, int32_t remap);

WASM_IMPORT("memcpy")
void memcpy_tic(uint32_t copyto, uint32_t copyfrom, uint32_t length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would tic_memcpy make more sense? I'm not sure, but this is one of the few pieces that caught my eye. I assume this is because of the C built-in memcpy function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it is due to conflicts with the C standard library. While tic_memcpy, would be the more normal way to say the function name, in practice when you want to use memcpy you would probably just start typing it without realizing tic_ should be in front, whereas it being treated as a suffix allows for IntelliSense to catch it.

Having it be a suffix too makes it feel less inconsistent. If two of the function names began with tic_, then it would be strange for not all of them to begin with this prefix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You make a good case...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out elsewhere C's own native memcpy would likely be sufficient - there isn't supposed to be any magic.... is tapping into that a possibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that was allowed for this, I assumed TIC's memcpy and memset were special.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They shouldn't be. The WASM VM has access to all the RAM - it should be able to move it about at at will . Really the memcpy and memset make a whole lot less sense when you're in C as you have directly access to RAM. Remember most people are using scripting runtime so the only way to get to the devices 96kb of MMIO is via these special functions, where-as in the WASM runtime all the memory is just freely available.

Copy link
Collaborator

@joshgoebel joshgoebel Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the API is different though, that's a sticky point... we'd prefer the same TIC API arguments, but if we used native memcpy behind the scenes I think that'd be fine. So is that doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have removed these TIC functions and tested the standard memset and it works fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just add a mention to the README that for simplicity and to avoid conflicts one should use the standard C memory functions rather than the TIC api ones...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the README. I think the wording is ok, but I'm not sure.

These functions are already part of C.
Gitignore you are killing me.
templates/c/README.md Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Collaborator

I reworded a bit... you happy with everything else?

@Gota7
Copy link
Contributor Author

Gota7 commented Jul 16, 2022

I reworded a bit... you happy with everything else?

I just updated the header with some fixes, I think it's good now.

Comment on lines 47 to 48
KEY_MINUS,
KEY_EQUALS,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent off?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, np.

@joshgoebel
Copy link
Collaborator

Lets just fix the indent and I think this is looking great! @Gota7 Thanks for all the assistance!

Copy link
Owner

@nesbox nesbox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your help 👍

@nesbox nesbox merged commit 66da4a7 into nesbox:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants