LacunaSpace / basicmac

BasicMAC LoRaWAN stack that supports (but is not limited to) Arduino
Other
79 stars 18 forks source link

Added ARDUINO_TTGO_LoRa32_V1 board and script for Windows environment #16

Closed pe1mew closed 3 years ago

pe1mew commented 4 years ago
  1. Added ARDUINO_TTGO_LoRa32_V1 board to standard-pinmaps.ino and verified operation to be working OK.
  2. Added export batch script for use in Windows environments to extract Arduino compatible library, similar to the shell script that is already available. The batch script will work when no spaces are part of the directory path.
pe1mew commented 4 years ago

I have rebased the fork and added the new batch file. A PR will follow.

pe1mew commented 4 years ago

Please evaluate changes and Pull when accepted.

matthijskooijman commented 4 years ago

I have rebased the fork and added the new batch file.

Seems you merged in my master branch, rather than rebasing on top of it, so now there's even more commits in the PR than less :-) Don't worry, though, I can just clean this up easily when merging.

A PR will follow.

Seems you discovered you can push more commits to updated this PR rather than create a new PR, which is also what I prefer. If you do rebase, you'll have to force push, but that will also work to update the PR (but replaces commits rather than just adding them).

The script installs into a BasicMAC subdirectory of the target directory (so you would pass your Arduino/libraries directory to the script), but for consistency with the Linux version of the script, I'd rather have it install into the given directory directly (so you'd pass Arduino/libraries/BasicMAC to the script). Seems I missed this on my first review, sorry.

You now implemented a "Are you sure" prompt for deleting the old directory. With that, it seems you're already quite close to making this script behave exactly like the Linux version (which would be preferred if possible). That would mean:

  1. If called with -h or --help, print usage as now (the linux version only knows --help, but I think I'll add -h there as well).
  2. If called with any other argument starting with -- print an "unknown option" error message (this one is not so important and can be omitted if needed)
  3. If the directory name passed does not exist, create it and copy files into it directly.
  4. If the directory name passed already exists, show a prompt and if the answer is yes, remove the existing file or directory, create a new directory and copy files into it.

This would mean dropping the --create and --replace arguments (I proposed the latter mostly since I wasn't sure how complex a "remove existing directory"-prompt would be, but apparently you've figured that one out already).

pe1mew commented 4 years ago

Git is not always clear to me, but we will get there eventually. I observed the 9 commits that I intended to have "removed" kind of.

With respect to your features: you would only like to have one option -h and the flow you described?

matthijskooijman commented 4 years ago

Git is not always clear to me, but we will get there eventually. I observed the 9 commits that I intended to have "removed" kind of.

It's a bit like black magic sometimes :-p

With respect to your features: you would only like to have one option -h and the flow you described?

Yup, then it will work identically between Linux and Windows (except for the missing --link symlink feature, which is probably hard or impossible on Windows).

pe1mew commented 4 years ago

Ok, done. The API and behaviour is now compatible with export.sh.

pe1mew commented 4 years ago

Hi Matthijs, did you finish assessing changes required for PR?

matthijskooijman commented 4 years ago

I had another look at the script, there's two questions still open above.

Also, looking again, I see that all directories are created and copied explicitly, including e.g. the individual example directories. This means that if e.g. an example is added, the script needs to be modified. Is there a way we can avoid that? i.e. do a recursive copy? Ideally, the individual mkdirs could also be removed when the copy creates each directory it copies to (leaving only a mkdir for the root and src directories, like in the bash script).

pe1mew commented 4 years ago
I had another look at the script, there are two questions still open above.

Great, it took a while.

Also, looking again, I see that all directories are created and copied explicitly, including e.g. the individual example directories. This means that if e.g. an example is added, the script needs to be modified. Is there a way we can avoid that? i.e. do a recursive copy? Ideally, the individual mkdirs could also be removed when the copy creates each directory it copies to (leaving only a mkdir for the root and src directories, like in the bash script).

I will close the PR, commit the changes and do another PR.

matthijskooijman commented 4 years ago

@pe1mew I noticed a few more things where your script behaved differently (i.e. export.sh expects the library directory itself to be passed, while your script expected the parent libraries directory). Rather than bouncing forward and back, I fired up Windows and played around with the script myself to fix the remaining incompatibilities, but ended up pretty much rewriting the script completely to follow the same interface and structure as the original export.sh.

The result in is #20, maybe you could see if that version of the batch script works for you and if it looks good?

matthijskooijman commented 3 years ago

I'm closing this, now that #20 has been merged.