Skip to content

Add ESP-IDF build targets#5334

Open
hectorchu wants to merge 4 commits into
tinygo-org:devfrom
hectorchu:esp-idf
Open

Add ESP-IDF build targets#5334
hectorchu wants to merge 4 commits into
tinygo-org:devfrom
hectorchu:esp-idf

Conversation

@hectorchu

Copy link
Copy Markdown
Contributor

This PR adds ESP-IDF build targets for esp32c3 and esp32s3.

This allows you to build a relocatable object file which can be bundled inside a static library archive and used as-is in ESP-IDF projects.

@b0ch3nski

Copy link
Copy Markdown
Contributor

This looks quite cool! Can you provide some example on how are you using this?

@hectorchu

Copy link
Copy Markdown
Contributor Author

Sure. I am linking a Go module into a regular ESP-IDF project.

If you want code, see:

The Go module entry point
https://github.com/ltcmweb/seedsigner/blob/micropy/src/mweb/mweb.go

The micropython binding to the Go module
https://github.com/ltcmweb/seedsigner-micropython-builder/blob/main/bindings/modmweb_bindings.c

@hectorchu

Copy link
Copy Markdown
Contributor Author

And the Go code runs in its own thread:
https://github.com/ltcmweb/seedsigner/blob/micropy/src/mweb/mweb.py

@b0ch3nski

Copy link
Copy Markdown
Contributor

Woah, I wasn't expecting also Python being involved here. So if I understand this correctly, you are 'exporting' Go module that is then called from Python using the C bridge, right? Could you share a reason behind this?

@hectorchu

Copy link
Copy Markdown
Contributor Author

Can you specify your question a bit clearer? Because right now my reasons for doing it this way is because the requirements demanded it. I was not willing to rewrite the entire Go module in C.

@aykevl aykevl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable to me (but see the comments below).

Others (@deadprogram etc) have an opinion on this? This doesn't seem too invasive of a change, though I'm not sure we can confidently support this across all ESP-IDF versions etc.

Also, looks like this needs to be rebased on dev to merge conflicts with #5453.

Comment thread targets/esp32s3-idf.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add esp32s3-idf to TestClangAttributes, and add a smoke test.

Comment thread targets/esp32c3-idf.json

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add esp32c3-idf to TestClangAttributes, and also add a smoke test to GNUmakefile.

Comment thread targets/esp32c3-idf.json
{
"inherits": ["riscv32"],
"features": "+32bit,+c,+m,+zmmul,-a,-b,-d,-e,-experimental-sdext,-experimental-sdtrig,-experimental-smctr,-experimental-ssctr,-experimental-svukte,-experimental-xqcia,-experimental-xqciac,-experimental-xqcicli,-experimental-xqcicm,-experimental-xqcics,-experimental-xqcicsr,-experimental-xqciint,-experimental-xqcilo,-experimental-xqcilsm,-experimental-xqcisls,-experimental-zalasr,-experimental-zicfilp,-experimental-zicfiss,-experimental-zvbc32e,-experimental-zvkgs,-f,-h,-relax,-sha,-shcounterenw,-shgatpa,-shtvala,-shvsatpa,-shvstvala,-shvstvecd,-smaia,-smcdeleg,-smcsrind,-smdbltrp,-smepmp,-smmpm,-smnpm,-smrnmi,-smstateen,-ssaia,-ssccfg,-ssccptr,-sscofpmf,-sscounterenw,-sscsrind,-ssdbltrp,-ssnpm,-sspm,-ssqosid,-ssstateen,-ssstrict,-sstc,-sstvala,-sstvecd,-ssu64xl,-supm,-svade,-svadu,-svbare,-svinval,-svnapot,-svpbmt,-svvptc,-v,-xcvalu,-xcvbi,-xcvbitmanip,-xcvelw,-xcvmac,-xcvmem,-xcvsimd,-xesppie,-xmipscmove,-xmipslsp,-xsfcease,-xsfvcp,-xsfvfnrclipxfqf,-xsfvfwmaccqqq,-xsfvqmaccdod,-xsfvqmaccqoq,-xsifivecdiscarddlone,-xsifivecflushdlone,-xtheadba,-xtheadbb,-xtheadbs,-xtheadcmo,-xtheadcondmov,-xtheadfmemidx,-xtheadmac,-xtheadmemidx,-xtheadmempair,-xtheadsync,-xtheadvdot,-xventanacondops,-xwchc,-za128rs,-za64rs,-zaamo,-zabha,-zacas,-zalrsc,-zama16b,-zawrs,-zba,-zbb,-zbc,-zbkb,-zbkc,-zbkx,-zbs,-zca,-zcb,-zcd,-zce,-zcf,-zcmop,-zcmp,-zcmt,-zdinx,-zfa,-zfbfmin,-zfh,-zfhmin,-zfinx,-zhinx,-zhinxmin,-zic64b,-zicbom,-zicbop,-zicboz,-ziccamoa,-ziccif,-zicclsm,-ziccrse,-zicntr,-zicond,-zicsr,-zifencei,-zihintntl,-zihintpause,-zihpm,-zimop,-zk,-zkn,-zknd,-zkne,-zknh,-zkr,-zks,-zksed,-zksh,-zkt,-ztso,-zvbb,-zvbc,-zve32f,-zve32x,-zve64d,-zve64f,-zve64x,-zvfbfmin,-zvfbfwma,-zvfh,-zvfhmin,-zvkb,-zvkg,-zvkn,-zvknc,-zvkned,-zvkng,-zvknha,-zvknhb,-zvks,-zvksc,-zvksed,-zvksg,-zvksh,-zvkt,-zvl1024b,-zvl128b,-zvl16384b,-zvl2048b,-zvl256b,-zvl32768b,-zvl32b,-zvl4096b,-zvl512b,-zvl64b,-zvl65536b,-zvl8192b",
"build-tags": ["espidf", "esp"],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since you are not defining esp32c3, I assume it is okay for this target to be entirely without support from the machine package?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a bit more documentation somewhere (this file, for example) describing how to use this target?

Comment on lines +69 to +70
var ts timespec
clock_gettime(clock, &ts)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that this might result in a heap allocation, which might not be intended (especially in a potentially time sensitive function).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, but how would I ensure the struct is allocated on the stack? Is there no escape analysis?

usleep(uint(d / 1e3))
}

const baremetal = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's sort of in between now: ESP-IDF is kind of like an OS.
But since it isn't actually used (only use is in gc_blocks.go and it doesn't matter since hasScheduler is also false) it may be worth setting to false since what this actually controls is interrupt handling in the GC.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you move this to a new file? (Note that as long as tinygo_scanCurrentStack is not called it will be removed from the binary by the linker).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think its because it still needs tinygo_scanCurrentStack for the GC. Without importing the other definitions in esp32s3.S which cause multiple definitions if I remember correctly.

Comment thread builder/build.go
libcDependencies = append(libcDependencies, libcJob)
libcDependencies = append(libcDependencies, makeMinGWExtraLibs(tmpdir, config.GOARCH())...)
case "":
case "", "none":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just keep it as an empty string?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work. You can't set the libc to an empty string in the target json, it doesn't override and unset any base target setting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because in the json, the empty string is omitted (because of omitempty), so the setting never gets set to the empty string.

@deadprogram

Copy link
Copy Markdown
Member

@hectorchu can yoou please resolve any merge conflicts? Thank you.

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.

4 participants