Skip to content

Conversation

@suda
Copy link
Contributor

@suda suda commented Feb 16, 2020

Tested on all three devices:

  • ✅ GPIO
  • ✅ UART
  • ✅ I2C

This PR also adds the ability to change the UART ports when calling Configure on nRF platform (used to test UART on different pins).

Flashed using Particle Debugger which works with cmsis-dap profile in OpenOCD.

@suda suda requested a review from aykevl February 16, 2020 15:21
Copy link
Member

@conejoninja conejoninja left a comment

Choose a reason for hiding this comment

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

It's another repo, but it would be great if the info about the boards is added to the tinygo.org website : https://github.com/tinygo-org/tinygo-site/tree/master/content/microcontrollers

@suda suda force-pushed the feature/particle-boards branch from 60c9fa2 to 612081a Compare February 17, 2020 14:28
@suda suda requested a review from conejoninja February 17, 2020 15:54
Copy link
Member

@conejoninja conejoninja left a comment

Choose a reason for hiding this comment

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

Boron and Xenon are missing the type "Pin" in some constants. Regular digital/analog pins are missing (A1, D8, D3, D2,...) in the three boards. Everything else looks good to me, I do not have the boards but I saw the tweet so I guess it works.

@suda
Copy link
Contributor Author

suda commented Feb 18, 2020

✅ Added the GPIO pins
✅ Fixed some incorrect pins in Boron
✅ Updated to support changes from #883 and set machine.Serial to the USB-CDC

Once #884 gets merged in, I can also set the correct Particle USB VID/PID

@conejoninja conejoninja self-requested a review February 18, 2020 11:36
Copy link
Member

@conejoninja conejoninja left a comment

Choose a reason for hiding this comment

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

Great! thanks for your contribution. One last thing, add them to the list of supported targets https://github.com/tinygo-org/tinygo/tree/dev#supported-boardstargets and I think we can merge this.

@conejoninja
Copy link
Member

We should add them to the tinygo website too https://github.com/tinygo-org/tinygo-site/tree/master/content/microcontrollers

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

There is one thing missing: smoke tests for these devices. Take a look here: https://github.com/tinygo-org/tinygo/blob/master/Makefile#L255-L256. These tests make sure the boards will continue to build with future changes to the machine package and in fact catch a lot of bugs in CI.

I have added a number of nitpicks below. They are optional to me, feel free to fix them or not.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One thing I realize just now is that you might want to use a custom linker script for this chip to preserve the bootloader and SoftDevice. See targets/nrf52840-s140v7.ld for an example.

@conejoninja
Copy link
Member

Good for me too, should we merge now and add the linker script later?

@aykevl
Copy link
Member

aykevl commented Feb 18, 2020

I'd like to know what @suda thinks before merging.

@suda
Copy link
Contributor Author

suda commented Feb 18, 2020

Oooh the script would be great but I agree with Daniel, we can decouple it into a separate PR.

@conejoninja conejoninja merged commit 0c0af6d into tinygo-org:dev Feb 18, 2020
@conejoninja
Copy link
Member

Thanks for this PR! Great job 👍 now merging

@suda suda deleted the feature/particle-boards branch February 18, 2020 22:53
@aykevl
Copy link
Member

aykevl commented Feb 20, 2020

@conejoninja for the next PR: to keep a clean history can you squash such commits? They're mostly going back and forth fixing things while the core change is really just one thing. Ideally it would have been one commit per board but I think that would be hard to change at a later time. A clean history is convenient for things like making a changelog and bisecting bugs.

@conejoninja
Copy link
Member

no problem! I always have doubts between rebase and squash, but I just left a note to myself to squash from now on. :)

@aykevl
Copy link
Member

aykevl commented Feb 20, 2020

There is no hard and fast rule, but in general I try to keep one commit per actual change:

  • Changes that make sense on their own should be kept in separate commits.
  • Commits that fix issues in previous commits in the same PR (after reviews etc.) are not independent changes and should ideally be squashed into those original commits.

Not everyone knows the git-fu to do such things so it's not a big problem, but it helps to keep the history clean when possible.

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