-
Notifications
You must be signed in to change notification settings - Fork 9
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 docs to Mix tasks #41
base: main
Are you sure you want to change the base?
Conversation
c7339fa
to
06751af
Compare
Looking good, I would back out the readme changes - pending other PR, or wait for it to land and rebase.. And then just the minor nitpicks.. Thanks! |
72daae7
to
d2b7dfc
Compare
The check task, if you want to document it verifies that the functions and modules used are either part of the application source (or deps) or supported by AtomVM. This will catch the use of any standard Elixir modules or functions used in the application that are not included in exavmlib. The packbeam task depends on this one, so users will likely never need to use it directly, but just in case they are wondering what it is doing a short module doc description wouldn’t hurt. |
d2b7dfc
to
fb9d347
Compare
lib/mix/tasks/packbeam.ex
Outdated
@shortdoc "Bundle the application into an AVM file" | ||
|
||
@moduledoc """ | ||
Bundle an application into an AVM file that can be flashed to a micro-controller and executed by the AtomVM virtual machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention running on generic_unix too?
...flashed to a micro-controller and (or directly on a unix host) executed by the AtomVM virtual machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this is turning into a bigger job than you probably anticipated, we all really appreciate your work here ;-). I just found a couple small details. Also if you change your PR description to so say "Closes #40" instead, the issue will automatically be closed when the PR is merged.
lib/mix/tasks/stm32_flash.ex
Outdated
> | ||
> Note. Before running this task, you must flash the AtomVM virtual machine to the target device. | ||
> | ||
> This tasks depends on a host installation of STM32 tooling, see [](https://www.atomvm.net/doc/main/build-instructions.html#building-for-stm32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the name for the link here. [STM32 Build Instructions]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice catch.
`atomvm.uf2create` task. | ||
|
||
* `:app_start` - The flash address ,in hexademical format, to place the application, default `0x10180000` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the :family_id
is also an important setting, but I just realized that #34 is still not merged yet, so the parameters and defaults for this setting will change. We can add that later, after we see which PR lands first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or create a new issue/PR later? I could pick it up when due.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely leave that for later. If #34 gets merged first that could be added here, or I can just as easily create a new PR to add documentation for that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such documenting tasks are low-hanging contribution apples to me. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But very appreciated! Documentation was one of the first areas I started making contributions in. It helped me learn a lot about how everything works. It is a big job to get everything documented and keep it accurate and up to date. Contributions of this kind are very valuable to the users, as well as developers - who often get caught up working on new features, or hunting down bugs. It's easy to not think about the documentation for months at a time ;-)
fb9d347
to
f61d985
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't catch this little detail sooner;-)
lib/mix/tasks/packbeam.ex
Outdated
Within your AtomVM mix project run | ||
|
||
` | ||
$ mix atomvm.packbeam --start MyProject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --start module option should only be used to override the :start defined in the :atomvm mix.exs. This would not normally be needed, unless the user had an alternate mode of operation... like a client/server app that normally builds the client, but when building the server uses a different start module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added. Also made the admonition blocks more prominent where applicable.
f61d985
to
ffaa819
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your fixes, brought up another little detail. But this is minor, so feel free to leave things as they are ;-)
|
||
For example, you can use the `--start` option to specify or override the `start` property. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we mentioned the dependencies elsewhere, this is actually another that users will not often need to use directly. The various MCU.flash tasks depend on this one, it is only for generic_unix, or WASM (?) that this task will be required for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow. But this is only an example that it is possible to set this flag if needed. Or do you mean that this flag is always needed when running this particular task directly? (I've no problem with changing copy if wished)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean the flag, I meant using the atomvm.packbeam task directly is not needed for micro-controllers. The atomvm.esp32.flash, atomvm.stm32.flash, and atomvm.pico.flash all depend on atomvm.packbeam, so using the packbeam task directly is only needed to create an AVM file for generic_unix... and possibly WASM, but I am not up to speed on deploying an Elixir WASM application, so I don't know all the details there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mentioned in the uf2create task that the atomvm.pico.flash task depends on uf2create task, so using it directly is not normally needed... I thought we might mention similarly about the atomvm.packbeam task.
ffaa819
to
84af458
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic! Just one little typo.
And we need you have you sign off on the commit. Looks like we need a workflow in place to check for that.
This is the very last little detail, I promise! ;-)
lib/mix/tasks/packbeam.ex
Outdated
|
||
> #### Info {: .info} | ||
> | ||
> Normally using this task manually is not required, it is called automatically by `atomvm.esp32.flash`, `atomvm.stm32.flash` nd `atomvm.pico.flash`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo nd -> and ;-)
84af458
to
9da76d0
Compare
To add a signoff to your commit you can use:
…or the short option |
9da76d0
to
6c4cc2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Should be ready to merge. Than you so much for all the effort and putting up with the nit-picking 😁
Let's back out mix.lock and it should be good to go. Adding it at this stage only creates issues, and solves none. (eg is this mix.lock valid on elixir 1.X), will it load outdated ex_doc, once we start using that etc. Surely we will add mix.lock once it's beneficial, but even then it may be sideloaded in Docs CI - since elixir 1.7-1.19 is theoretically supported, and I foresee a mix.lock conflicting with that, I could just be imagining that, but I'm not gonna investigate. |
6c4cc2b
to
14ceda8
Compare
mix.exs
Outdated
@@ -21,7 +32,8 @@ defmodule ExAtomVM.MixProject do | |||
# Run "mix help deps" to learn about dependencies. | |||
defp deps do | |||
[ | |||
{:uf2tool, "1.1.0"} | |||
{:uf2tool, "1.1.0"}, | |||
{:ex_doc, "~> 0.34", only: :dev, runtime: false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"~> 0.34"
-> "~> 0.20"
for maximum compatibility with anybody running old elixir versions..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
great work @joustava - thank you, and apologies for things not being straightforward... |
- mix help will now list the available atomvm task - mix doc will generate ex docs Signed-off-by: Joost Oostdijk <joustava@gmail.com>
14ceda8
to
3102630
Compare
Made changes to the Mix task implementations so that
Am not sure how to word the moduledoc for the check task though, feedback welcome.
Closes #40