Board-based instantiation of chip drivers and interrupt mappings for nordic boards/chips #2084

hudson-ayers · 2020-08-25T02:35:35Z

Pull Request Overview

This pull request applies the new peripheral instantiation approach proposed in #2069 to the nrf52840 and nrf52832 chips and associated boards. These chips were already hierarchically structured, so I applied that same approach to the macro used to create the default peripherals. Also, the nordic chips have some peripherals (such as USBD and POWER) which depend on each other, so I had to add an additional init() function to allow for finalizing these circular dependencies after instantiation of the drivers.

~~This PR sits on top of #2069, which is why it is so large.~~

Testing Strategy

This pull request was tested by compiling, and by running the following apps on the nrf52840dk (several at a time):

blink
ieee802154/radio_tx
buttons
ble_advertising
rng
nonvolatile_storage
c_hello

TODO or Help Wanted

N/A

Documentation Updated

No updates are required.

Formatting

Ran make prepush.

gendx

I had a look only at the Nordic-specific parts. I have two main concerns with this design.

The new peripherals APIs seem to violate Rust's memory safety rules. Whereas an unsafe block was required before to access them (due to static mut variables being unsafe to access), there are now public new() functions exposed that are not marked as unsafe. I think they should be unsafe, as one must only be allowed to create a single peripheral in the program to avoid aliasing registers.
Besides an unclear naming, I don't see what the create_default_foo_peripherals macros are trying to achieve. The $N parameter is not useful (for Nordic at least), as each macro is always used with the same name. Can't the corresponding structures be defined once in the corresponding chip (without any macro), and then have the boards simply use the structures to import them in the current namespace, if that's the goal? I think macros should generally be avoided as they make code less clear (similarly to Cargo features), except for really useful cases like register_structs or static_init.

Besides that, would it make sense to define a Peripheral trait, that would declare the new function? That is:

trait Peripheral {
    const unsafe fn new() -> Self;
}

Or are traits too restrictive regarding const and/or unsafe?

gendx · 2020-08-27T10:01:07Z

boards/nordic/nrf52840_dongle/src/io.rs

+        // Here, we create a second instance of the Uarte struct.
+        // This is okay because we only call this during a panic, and
+        // we will never actually process the interrupts
+        let uart = nrf52840::uart::Uarte::new();


Does this not require unsafe?

currently no. We could consider making it take a 'PeripheralCreationCapability'.

gendx · 2020-08-27T10:01:46Z

boards/nordic/nrf52840_dongle/src/main.rs

@@ -56,7 +56,8 @@ static mut PROCESSES: [Option<&'static dyn kernel::procs::ProcessType>; NUM_PROC
    [None; NUM_PROCS];

 // Static reference to chip for panic dumps
-static mut CHIP: Option<&'static nrf52840::chip::Chip> = None;
+nrf52840::create_default_nrf52840_peripherals!(Nrf52840Peripherals);


How can this be invoked outside of a function?

It only defines a struct and methods on that struct, it doesn't do anything at runtime.

Ok, see my other comment regarding the naming. Let's discuss that there.

boards/nordic/nrf52840_dongle/src/main.rs

gendx · 2020-08-27T10:07:45Z

chips/nrf52/src/acomp.rs

@@ -217,9 +217,9 @@ pub struct Comparator<'a> {
 }

 impl<'a> Comparator<'a> {
-    const fn new(registers: StaticRef<CompRegisters>) -> Self {
+    pub const fn new() -> Self {


How are such new() functions not marked as unsafe? One can instantiate them twice with safe code, which then aliases the registers, violating Rust's fundamental memory safety rules. On the contrary, the previous code only exposed static mut variables as public code, and static mut required unsafe to use.

I think we should come up with a better API.

I spoke with @alevy about this, and I do not believe it violates Rust's memory safety rules. StaticRef provides access to the underlying registers by wrapping the underlying registers in an UnsafeCell, but it is not memory unsafe for multiple references to a StaticRef to exist. Notably, this design does expose a footgun in that it is possible to instantiate a peripheral twice, when only one can be hooked up to receive interrupts. But that is actually occasionally useful (for the UART in the panic handler, for example). We could have each new function require a capability to lessen the impact of this footgun, but I am not sure if that is necessary.

Note that within a given module it was already possible to alias the registers without unsafe by simply using the const BASE_REGISTERS multiple times, which did not require unsafe. In fact, there are already instances of code that does so in Tock drivers today.

That being said, I would be happy to hear suggestions for an alternate API.

StaticRef provides access to the underlying registers by wrapping the underlying registers in an UnsafeCell.

I don't see any UnsafeCell https://github.com/tock/tock/blob/master/kernel/src/common/static_ref.rs. But after giving it a second look, the StaticRef API indeed looks fine to me.

There is an unsafe function to create a StaticRef, the prerequisite being a non-null pointer with 'static lifetime to a T object that is otherwise not aliased in the program. I don't see these prerequisites violated in the peripherals we instanciate.

Cloning is safe. No issue with that, cloning a pointer is in itself safe.

Dereferencing to a &'static T. This is safe because the pointer passed to new has 'static lifetime, and indeed points to a T (in particular, is aligned correctly). It's also fine to dereference clones of a StaticRef, as we obtain immutable references.

The UnsafeCell part is from https://github.com/tock/tock/blob/master/libraries/tock-register-interface/src/registers.rs. This also looks fine to me, w.r.t. the UnsafeCell rules (https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html).

The register API doesn't expose any &T or &mut T from the UnsafeCell in register. Simple set/get API.

No multi-threading in Tock.

I wonder however how that plays with re-entrancy. In normal operation the Tock scheduler doesn't call code re-entrantly (just queues pending interrupts for the main loop to process).

The panic handler path should be fine, but is not 100% clear to me. What if a panic happens from within the UART code, and that we re-entrantly call UART code again?

Note that within a given module it was already possible to alias the registers without unsafe by simply using the const BASE_REGISTERS multiple times, which did not require unsafe. In fact, there are already instances of code that does so in Tock drivers today.

Good point.

So I think I don't have much concerns left here. In short, it's possible to instantiate a set of registers twice, and have the two instances make unrelated writes to the underlying registers (but not concurrently in the multi-threaded sense, as Tock is single-threaded), which would certainly violate the logic of the underlying peripheral, but wouldn't trigger any UB at the Rust language level (so there is no risk that compiler optimizes away or things like that).
I think it's fine, and best to keep unsafe outside of that (I'm all for reducing the amount of unsafe code to the minimum).

But I still want to note that depending on what the peripheral does of it, logic bugs where a peripheral's registers are put in an inconsistent state (due to multiple instantiations of the same registers) may still trigger very bad stuff (though I don't have a specific example in mind). So it would be nice to have a way to prevent instantiating registers twice (except for those required in panic handlers), to avoid logic bugs, but that's out of this pull request's scope.

Thanks for looking into this some more! Agreed about the potential for logic bugs, but I don't think its that much worse than before where there was widespread use of the globals that allowed for the same bugs, just requiring an unsafe.

There are some good use cases where it is useful to have multiple instances of a set of registers.

For example with a pin controller. Let's say there is a pin controller that can be used to allow either I2C or SPI. You could pass the pin controller registers to both the I2C and the SPI devices and then have the I2C/SPI device configure the pins before it starts. Obviously you would then need to make sure only 1 is operating at a time (maybe with a virtual_bus capsule).

The same sort of thing would apply to power management, where each device can enable and disable itself as it is started/stopped. For the Apollo3 for example it would be great to only enable devices when accessed, instead of just enabling everything at boot. Stopping devices is another problem that is much more complex.

This can be done by making the const XXX: StaticRef<Registers> --> pub const XXX: StaticRef<Registers> instead. But I don't think we want to take that approach universally, as it will often only make sense when there is some virtualization ensuring concurrent accesses do not lead to unexpected behavior.

For power management the state-of-the-art in Tock previously was anything that wanted to manage power accessing some global PowerManager struct. This change at least requires all devices which want to use the struct to take a reference to the power manager when they are instantiated, so it is clearer which peripherals will interact with it.

gendx · 2020-08-27T10:10:24Z

chips/nrf52/src/chip.rs


-pub struct NRF52<I: InterruptService> {
+pub struct NRF52<I: InterruptService<DeferredCallTask> + 'static> {


Why is a reference now needed? And if we're introducing new references, can we use a generic 'a lifetime parameter, in the spirit of #1074?

A reference is now needed because the struct that implements the interrupt service now also holds all the peripherals, and there are instances where the peripherals need to be instantiated before the Chip struct itself (maybe not for the nordic boards, but I think its good to be consistent across all chips).

I changed it to use a generic lifetime.

gendx · 2020-08-27T10:13:54Z

chips/nrf52/src/chip.rs

+/// constructed manually in main.rs. The input to the macro is the name of the struct
+/// that will hold the peripherals, which can be chosen by the board.
+#[macro_export]
+macro_rules! create_default_nrf52_peripherals {


I'm not sure that "create" is the clearest name for this macro. Without context, I had assumed that the contents of the macro were some code that initializes the peripherals, to put inside a function (similarly to static_init). I didn't assume the macro's contents were a struct definition.

Can we come up with a better naming?

How about define_default_nrf52_peripherals?

gendx · 2020-08-27T10:15:36Z

chips/nrf52/src/chip.rs

+/// that will hold the peripherals, which can be chosen by the board.
+#[macro_export]
+macro_rules! create_default_nrf52_peripherals {
+    ($N:ident) => {


Why is this name a parameter? I only see the macro invoked with "Nrf52BasePeripherals".

It doesn't really have to be a parameter. I made it a parameter so that if someone wanted to switch between the macro version and a version with fewer drivers instantiated in boards, it would be a little easier. But I am fine with a version that does not pass a name.

hudson-ayers · 2020-08-27T14:47:52Z

Besides an unclear naming, I don't see what the create_default_foo_peripherals macros are trying to achieve. The $N parameter is not useful (for Nordic at least), as each macro is always used with the same name. Can't the corresponding structures be defined once in the corresponding chip (without any macro), and then have the boards simply use the structures to import them in the current namespace, if that's the goal? I think macros should generally be avoided as they make code less clear (similarly to Cargo features), except for really useful cases like register_structs or static_init.

Agreed that the naming parameter is not that useful, I can get rid of that. I also agree that it would be possible to take the approach you describe that does not require a macro. However, the main motivation of this change is that it allows for out-of-tree boards to selectively exclude peripherals they do not need. To do so, we need to ensure that all chip peripherals and interrupt mappings can be instantiated from within main.rs. By using a macro, we enforce that even in upstream boards, these things are executed in main.rs, so it is easy to see that out-of-tree boards could exclude peripherals and do the same. If we moved to an approach where upstream boards just use the structures defined in the chip, it would be easy to end up with peripherals that do not expose constructors outside the chip crate, but would still work for the in tree boards. Also, I think the macro approach makes it clear precisely what code should be copied into main.rs to create a more limited out-of-tree board.

Besides that, would it make sense to define a Peripheral trait, that would declare the new function? That is:
trait Peripheral {
    const unsafe fn new() -> Self;
}
Or are traits too restrictive regarding const and/or unsafe?

Traits are currently too restrictive regarding const. We could make new() not const with this new approach that does not rely on globals, however, which would make this possible.

gendx · 2020-08-28T09:04:23Z

To do so, we need to ensure that all chip peripherals and interrupt mappings can be instantiated from within main.rs. By using a macro, we enforce that even in upstream boards, these things are executed in main.rs, so it is easy to see that out-of-tree boards could exclude peripherals and do the same.

I don't really see the point here. Nothing is instantiated within main.rs, but simply defined (as we discuss in the other comment). I don't see how defining things in the main module provides anything better than defining them in a chip module, and use them in main.

If a custom board wants to exclude peripherals, they can define their own functions instead of using the chip's defaults. So I don't see what the macro provides that can't be done with simple definitions and use.

Do you have an example of an out-of-tree board and how it would leverage the macro?

hudson-ayers · 2020-08-28T14:20:53Z

I don't really see the point here. Nothing is instantiated within main.rs, but simply defined (as we discuss in the other comment). I don't see how defining things in the main module provides anything better than defining them in a chip module, and use them in main.

If a custom board wants to exclude peripherals, they can define their own functions instead of using the chip's defaults. So I don't see what the macro provides that can't be done with simple definitions and use.

Do you have an example of an out-of-tree board and how it would leverage the macro?

An out of tree board would not leverage the macro, it would simply copy past the code that currently resides within the macro into its own main.rs, and remove the peripherals it does not want.

Using a macro makes it so upstream boards define the peripherals struct in main as well, ensuring that the visibility of all structs and functions necessary to define peripherals and interrupt mappings can be accessed from a dependent crate (the board crate). If we moved to a non-macro definition, that was just imported into main, I worry about a scenario like this:

//! chips/apollo3/src/chip.rs

struct DefaultApollo3Peripherals {
    stimer: crate::stimer::STimer<'static>,
    // ...
}
impl DefaultApollo3Peripherals {
    unsafe fn new() -> Self {
        Self {
            stimer: crate::stimer::STimer::new(),
            // ...
        }
    }
}
impl kernel::InterruptService<()> for DefaultApollo3Peripherals {
    unsafe fn service_interrupt(&self, interrupt: u32) -> bool {
        use crate::nvic;
        match interrupt {
            nvic::STIMER..=nvic::STIMER_CMPR7 => self.stimer.handle_interrupt(),
            // ...

//! chips/apollo3/src/stimer.rs

pub struct STimer<'a> {
    registers: StaticRef<STimerRegisters>,
    client: OptionalCell<&'a dyn hil::time::AlarmClient>,
}

impl<'a> STimer<'_> {
    pub(crate) const fn new() -> STimer<'a> { // <-- Works for boards that use the struct in chips/, but cant call outside chips/
        STimer {
            registers: STIMER_BASE,
            client: OptionalCell::empty(),
        }
    }
}

Also, in this scenario it is somewhat less straightforward to paste the struct definition into main and edit it, because the board author has to modify all the paths to be from the board crate instead of from within the chip crate.

Does that make sense? I agree that macros are non-ideal, but I think if we use the non-macro approach here it will inevitably be the case that visibility will not be sufficient for out of tree board authors to only define an out of tree board crate, forcing them to maintain an out of tree chip crate as well.

chips/nrf52/src/chip.rs

gendx · 2020-08-31T09:17:37Z

So as I understand it, the macro would allow to circumvent the visibility rules to make it easier to support out-of-tree boards/chips. I think this raises multiple questions.

Regarding this specific problem: if peripherals have to be available outside of the chip crate, why not simply make them public? It would be much simpler to define appropriate visibility rules than trying to circumvent them with macros. In other words, pub const fn new() -> STimer<'a>.
More broadly, should Tock aim to support out-of-tree boards/chips? I'd argue that if they are out-of-tree, they are unsupported by default, and Tock shouldn't go to great lengths to support them. It doesn't mean that Tock should make it hard to integrate out-of-tree boards/chips, but I don't think out-of-tree boards/chips should steer Tock towards complex designs that are harder to maintain (e.g. features, macros, etc.).
I don't see a big issue in out-of-tree board maintainers having to also maintain out-of-tree chips, if the chip in question differs from the default in Tock. Also, how much maintenance is that really? If it's a simple git patch to apply on the chip crate I don't think it's that much of a burden for unsupported boards.
If maintaining out-of-tree boards is too difficult, shouldn't Tock have more boards/chips in-tree? In the past year, we've already added quite a few more boards/chips in Tock, so I don't think adding more of them is a problem per se. Actually, having more supported boards allows to make sure that core changes in the kernel are suited for a wide range of use cases (rather than forcing maintainers of out-of-tree boards to figure it out).
Of course, I don't think we should remove the possibility of defining out-of-tree boards, but these will necessarily have a maintenance burden for whoever manages them.

hudson-ayers · 2020-08-31T17:55:38Z

So as I understand it, the macro would allow to circumvent the visibility rules to make it easier to support out-of-tree boards/chips. I think this raises multiple questions.

Regarding this specific problem: if peripherals have to be available outside of the chip crate, why not simply make them public? It would be much simpler to define appropriate visibility rules than trying to circumvent them with macros. In other words, pub const fn new() -> STimer<'a>.

The goal of using a macro is to ensure that the peripherals are public. This PR makes all constructors public, but if they are not instantiated via a macro this will not be enforced in CI, and I suspect that future PRs will add peripherals with non-public constructors (or with constructors that rely on access to other non public fields), because doing so will not cause compilation failures for boards that use the default (which all upstream boards will).

More broadly, should Tock aim to support out-of-tree boards/chips? I'd argue that if they are out-of-tree, they are unsupported by default, and Tock shouldn't go to great lengths to support them. It doesn't mean that Tock should make it hard to integrate out-of-tree boards/chips, but I don't think out-of-tree boards/chips should steer Tock towards complex designs that are harder to maintain (e.g. features, macros, etc.).

I think this is a reasonable point, and we could argue that it is up to out-of-tree to submit patches if peripherals are mistakenly made not public. This would allow us to use this approach, without a macro.

I don't see a big issue in out-of-tree board maintainers having to also maintain out-of-tree chips, if the chip in question differs from the default in Tock. Also, how much maintenance is that really? If it's a simple git patch to apply on the chip crate I don't think it's that much of a burden for unsupported boards.

One point of concern is that rather than using patches, many out of tree boards (such as opentitan and the h1) currently maintain their own version of the chip crate entirely. This makes it harder for out of tree boards to stay up to date with the upstream chip crate, and makes out of tree boards less likely to submit bug fixes upstream, as the bug fixes are not widely available. Even OpenSK currently has outstanding bug fixes for the nrf52 USB driver that have not been submitted to upstream. Personally, I do think their is value to Tock in reducing the friction required to maintain boards out of tree.

If maintaining out-of-tree boards is too difficult, shouldn't Tock have more boards/chips in-tree? In the past year, we've already added quite a few more boards/chips in Tock, so I don't think adding more of them is a problem per se. Actually, having more supported boards allows to make sure that core changes in the kernel are suited for a wide range of use cases (rather than forcing maintainers of out-of-tree boards to figure it out).

Unfortunately there is a substantial burden to Tock developers to maintain more boards/chips in-tree, which is part of why we also deprecate little-used board/chip combos regularly. Regardless, I think a change to what boards Tock maintains upstream is a bit out of scope of this PR.

alistair23 · 2020-08-31T18:04:11Z

+1 for not using macros. It doesn't seem unreasonable to just have out of tree board maintainers submit fixes to mainline Tock to fix any non public peripherals. Hindering everyone else for a few out of tree users doesn't seem like the right approach. The out of tree users should just upstream their work.

hudson-ayers · 2020-08-31T18:17:58Z

Okay, I think I am convinced. I will try to update both PRs to use a non-macro approach, and add some docs updates to indicate that chips should keep peripherals publicly exposed to support out-of-tree boards.

hudson-ayers · 2020-09-09T06:47:08Z

I got rid of the large macros in favor of structs (and did so on the apollo3/sam4l version of this PR as well).

gendx

There are still a few pending comments, but apart from that it generally looks fine (for Nordic boards/chips at least). One question is whether this needs to be reviewed & submitted as a single pull request or split by board/chip groups (I don't mind either way).

chips/nrf52/src/chip.rs

chips/nrf52832/src/interrupt_service.rs

gendx · 2020-09-09T14:48:04Z

chips/nrf52840/src/interrupt_service.rs

+        self.nrf52_base.pwr_clk.set_usb_client(&self.usbd);
+        self.usbd.set_power_ref(&self.nrf52_base.pwr_clk);


This is fine, but I wonder whether these two calls cannot be bundled in one?

It could be done with a single (non-method) function, but would require changing the visibility of fields of one of the structs. I think this is more in line with how we set up circular dependencies elsewhere anyways.

chips/nrf52840/src/interrupt_service.rs

kernel/src/platform/mod.rs

gendx · 2020-09-09T14:51:03Z

kernel/src/platform/mod.rs

+/// where the kernel instructs the `nrf5` crate to handle interrupts, and if
+/// there is an interrupt ready then that interrupt is passed through the crates
+/// until something can service it.
+pub trait InterruptService<T> {


Shouldn't T be bounded by some kind of "task" trait?

That would make sense, but we don't have any DeferredCallTask trait today.

Shouldn't we introduce one with this pull-request then?

If we always use T = DeferredCallTask for now, I don't see the need for it to be generic.

If we use various task types, I think we should unify them with a trait.

Each chip defines its own Task type (sam4l calls it Task, nrf52 calls it DeferredCallTask), so it has to be a generic or an associated type for it to be part of the InterruptService trait definition. Perhaps an associated type would be better?

Currently, each Task type is just an enum that specifies different task types and can be converted to/from usize. Given that there are no common functions implemented across all Task types I am not sure how much value is gained from making a trait with no functions.

Would adding the last line of the following code snippet really help that much with unifying?

/// A type of task to defer a call for #[derive(Copy, Clone)] pub enum DeferredCallTask { Nvmc = 0, } impl DeferredTaskTrait for DeferredCallTask {}

I think such a marker trait would be useful, in restricting what kind of type can be applied there. Besides, when someone implements a new chip, they can look where DeferredTaskTrait is already implemented and more quickly understand what to do.

chips/nrf52832/src/interrupt_service.rs

chips/nrf52840/src/interrupt_service.rs

hudson-ayers · 2020-09-18T23:41:16Z

Rebased on top of the changes in #2069

hudson-ayers · 2020-11-05T06:35:39Z

Rebased on master. One change I had to make in capsules was to make the controller() function on the CdcAcm struct be pub, so that the nano33 could still easily access it from the panic handler without requiring two globals.

If anyone thinks that is problematic I could go with the 2 globals approach instead, but I think the way I chose is cleaner.

hudson-ayers requested a review from gendx August 25, 2020 02:35

gendx suggested changes Aug 27, 2020

View reviewed changes

gendx reviewed Aug 31, 2020

View reviewed changes

chips/nrf52/src/chip.rs Outdated Show resolved Hide resolved

gendx reviewed Sep 9, 2020

View reviewed changes

gendx reviewed Sep 14, 2020

View reviewed changes

chips/nrf52832/src/interrupt_service.rs Outdated Show resolved Hide resolved

gendx reviewed Sep 14, 2020

View reviewed changes

chips/nrf52840/src/interrupt_service.rs Outdated Show resolved Hide resolved

hudson-ayers force-pushed the no-more-globals-nrf52 branch from 6eb2f61 to cb37a66 Compare September 18, 2020 23:40

hudson-ayers mentioned this pull request Sep 23, 2020

Board based instantiation of chip drivers and interrupt mappings: Earlgrey/Opentitan #2122

Merged

2 tasks

hudson-ayers mentioned this pull request Oct 12, 2020

Board based instantiation of chip drivers and interrupt mappings: Arty_e21 #2153

Merged

2 tasks

bradjc previously approved these changes Nov 4, 2020

View reviewed changes

bradjc added the needs-rebase label Nov 4, 2020

hudson-ayers added 10 commits November 4, 2020 17:03

new peripheral instantiation model for nrf52840

4bd1ffa

update peripheral instantiation for all other nordic boards

82e1a24

move interrupt service documentation to kernel crate

8829b83

revert Makefile change

39c5506

revert process.rs change

768ed2e

remove unneeded braces

e72f89b

use generic lifetime for nrf52 interrupt service

070d6bb

no more macros nordic chips/boards

e382bf1

update InterruptService description

7b81fb7

nrf52_base --> nrf52

33efc90

hudson-ayers added 4 commits November 4, 2020 21:52

dont match on bool

666820f

propogate unsafe to main

7f4dd5a

dont match on bool, redux

2b1526b

add note about why nrf52 port is not entirely updated to the new model

5c9f824

hudson-ayers dismissed bradjc’s stale review via 5c9f824 November 5, 2020 06:32

hudson-ayers force-pushed the no-more-globals-nrf52 branch from 076241b to 5c9f824 Compare November 5, 2020 06:32

bradjc approved these changes Nov 5, 2020

View reviewed changes

bradjc removed the needs-rebase label Nov 5, 2020

bradjc merged commit 7afdbe8 into tock:master Nov 5, 2020


		pub struct NRF52<I: InterruptService> {
		pub struct NRF52<I: InterruptService<DeferredCallTask> + 'static> {

		self.nrf52_base.pwr_clk.set_usb_client(&self.usbd);
		self.usbd.set_power_ref(&self.nrf52_base.pwr_clk);

Board-based instantiation of chip drivers and interrupt mappings for nordic boards/chips #2084

Board-based instantiation of chip drivers and interrupt mappings for nordic boards/chips #2084

Conversation

hudson-ayers commented Aug 25, 2020 • edited

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

gendx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudson-ayers commented Aug 27, 2020

gendx commented Aug 28, 2020

hudson-ayers commented Aug 28, 2020

gendx commented Aug 31, 2020

hudson-ayers commented Aug 31, 2020

alistair23 commented Aug 31, 2020

hudson-ayers commented Aug 31, 2020

hudson-ayers commented Sep 9, 2020

gendx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudson-ayers commented Sep 18, 2020

hudson-ayers commented Nov 5, 2020

hudson-ayers commented Aug 25, 2020 •

edited