make: improve handling of the stdio_rtt module, improve dependency management at application level #12724

aabadie · 2019-11-16T13:48:18Z

Contribution description

This PR introduces the stdio_rtt feature for boards for which stdio is accessed via RTT (there is a strong dependency on this for these boards): hamilton, thingy52 and ruuvitag.

The (new) stdio_rtt feature is required when the (already existing) stdio_rtt module is used.
This then allows to blacklist this feature when stdio_ethos is used in the build: indeed stdio_ethos has a strong dependency to stdio_uart, which is not compatible.

Thanks to this, the PR also removes all BOARD_BLACKLIST variables that were added because of this incompatibility.

There is a small trick for gnrc_* tests application that was taken from #12304 (commit 6f43ab7 is cherry-picked from this PR in fact).

Some details (pic32 chronos boards removed) are explained in related commit details comments.

Testing procedure

A green Murdock
Check the list of board supported by updated applications

Issues/PRs references

None

miri64 · 2019-11-16T14:30:17Z

This logic seems reverse to me... shouldn't stdio_ethos in its dependencies rather add a FEATURE_REQUIRED += stdio_uart? This way there is no need to add FEATURE_BLACKLIST line to the applications in the first place?

RIOT/Makefile.dep

Lines 432 to 436 in d755d50

    
           ifneq (,$(filter stdio_ethos,$(USEMODULE))) 
        
             USEMODULE += ethos 
        
             USEMODULE += stdin 
        
             USEMODULE += stdio_uart 
        
           endif

aabadie · 2019-11-16T16:36:47Z

This logic seems reverse to me... shouldn't stdio_ethos in its dependencies rather add a FEATURE_REQUIRED += stdio_uart?

And where would you provide the stdio_uart features ? In the boards!

This way there is no need to add FEATURE_BLACKLIST line to the applications in the first place?

I prefer having to blacklist the feature provided by the 3 boards that are using Segger RTT for stdio instead of requiring a feature that will have to be provided by all the other ones.

aabadie · 2019-11-16T16:46:38Z

I fixed the failures on Murdock: on thingy52 and ruuvitag, there was an USEMODULE += stdio_rtt in a Makefile.include. This was breaking the features requirement resolution used by info-boards-supported. Moving this line in each boards' Makefile.dep (where this should be actually) fixed the issue :)

miri64 · 2019-11-17T10:35:20Z

This logic seems reverse to me... shouldn't stdio_ethos in its dependencies rather add a FEATURE_REQUIRED += stdio_uart?

And where would you provide the stdio_uart features ? In the boards!

Currently USEMODULE+= stdio_uart is set, when newlib is used and when neither stdio_cdc_ecm nor stdio_rtt are used:

RIOT/Makefile.dep

Lines 406 to 408 in 61c6b07

    
           ifeq (,$(filter stdio_rtt stdio_cdc_acm,$(USEMODULE))) 
        
             USEMODULE += stdio_uart 
        
           endif

Does anything speak against replacing USEMODULE with FEATURE_PROVIDED there?

This way there is no need to add FEATURE_BLACKLIST line to the applications in the first place?

I prefer having to blacklist the feature provided by the 3 boards that are using Segger RTT for stdio instead of requiring a feature that will have to be provided by all the other ones.

I prefer it if you don't have to think about it and it happens all automatically in the background (which was what I proposed). This is how the FEATURE system was supposed to work. To me FEATURE_BLACKLIST is just a last resort if anything else doesn't make sense. In this case having FEATURE_REQUIRED += stdio_uart as dependency for stdio_ethos makes total sense.

aabadie · 2019-11-17T11:49:03Z

I'm aware of how stdio_% dependencies work. An I have some ideas of how all this should be improved. But I think this is out of the scope of this PR.

Does anything speak against replacing USEMODULE with FEATURE_PROVIDED there?

It seems there's a confusion on your side between FEATURES_PROVIDED and FEATURES_REQUIRED. While the former is set by cpus or boards (and soon by drivers I hope ;) ), e.g. in Makefile.features files, the latter is set when managing dependencies, e.g. in Makefile.dep files. So your question should rather be "why not use FEATURES_REQUIRED instead of USEMODULE ?" Even if I also thought of it, I haven't tested because I thought it might not work. I give this is a try (only tomorrow) anyway, just to be sure.

Note that stdio_rtt is a real module, see here. stdio_uart as well. So I don't know how the build system will behaves if this is changed this way.

aabadie · 2019-11-17T11:50:41Z

I prefer it if you don't have to think about it and it happens all automatically in the background (which was what I proposed). This is how the FEATURE system was supposed to work.

This is exactly what this PR is trying to do. Using FEATURES_BLACKLIST is simpler and less intrusive than having to deal with a provided stdio_uart feature, like you suggested.

aabadie · 2019-11-18T07:44:22Z

I've reworked the management of dependencies with the introduced stdio_rtt feature. I think it's better now.

To convince @miri64, I'll try to better answer to your questions:

shouldn't stdio_ethos in its dependencies rather add a FEATURE_REQUIRED += stdio_uart?

stdio_uart is used by almost all boards in RIOT, except the 3 ones modified by this PR. If we introduce a feature for this, we would have to add it to the FEATURES_PROVIDED variable in all those boards (or common boards code), e.g. in their Makefile.features file. The management of how to require the feature: either in each board or globally in the main Makefile.dep is still to be determined (I have some ideas here but need to test).
Although I agree that would be more natural, this is a much more larger change as it's almost touching all boards.
Introducing here the stdio_rtt and using the FEATURES_BLACKLIST allows to go right to the point, for now, and only modify 3 boards, instead of 100+.

Does anything speak against replacing USEMODULE with FEATURE_PROVIDED there?

Even if there's a confusion between FEATURE_PROVIDED, FEATURE_REQUIRED and FEATURE_USED in this question, there's a simple answer: if a feature is used and there's a module with the same name, the dependency resolution simply add the module to the build if the feature is used (e.g it's provided and required, but not blacklisted). See how this is done with periph:

RIOT/Makefile.dep

Lines 987 to 989 in cd0b975

    
           # all periph features correspond to a periph submodule 
        
           # FEATURES_USED is defined in Makefile.features 
        
           USEMODULE += $(filter periph_%,$(FEATURES_USED))

This is basically what's done now between the new stdio_rtt feature and the corresponding module (which already exists).

fjmolinas · 2019-11-18T09:45:00Z

This logic seems reverse to me... shouldn't stdio_ethos in its dependencies rather add a FEATURE_REQUIRED += stdio_uart? This way there is no need to add FEATURE_BLACKLIST line to the applications in the first place?

I do not think this needs a FEATURE_REQUIRED += stdio_uart or FEATURE_PROVIDED += stdio_uart, we already have FEATURE_PROVIDED += periph_uart, IMO this is a configuration issue, if stdio_rtt is not compatible with ethos then maybe it should be DEFAULT_MODULE += stdio_rtt and disable it when using ethos.

Not sure if this works straight out of the box for those boards, might need some refactoring.

aabadie · 2019-11-18T09:47:35Z

Not sure if this works straight out of the box for those boards, might need some refactoring.

Just tested this locally and it works like a charm. I like your proposition @fjmolinas. Thanks!

aabadie · 2019-11-18T10:08:21Z

Adapted this PR following @fjmolinas's suggestion. By default the stdio_rtt module is used, except when stdio_uart is explictly added as a dependency, which is the case with ethos.

Since hamilton doesn't provide periph_uart, it's automatically excluded from the build of applications using ethos. For thingy52 and ruuvitag, stdio_uart is built but that means an USB to UART adapter must be connected to run the test applications on these platforms in this case.

miri64 · 2019-11-18T13:12:50Z

I do not think this needs a FEATURE_REQUIRED += stdio_uart or FEATURE_PROVIDED += stdio_uart, we already have FEATURE_PROVIDED += periph_uart, IMO this is a configuration issue, if stdio_rtt is not compatible with ethos then maybe it should be DEFAULT_MODULE += stdio_rtt and disable it when using ethos.

periph_uart and stdio_uart are not the same thing... A board that has stdio_rtt still can have a UART...

tests/gnrc_ipv6_ext/Makefile

aabadie · 2019-11-18T13:21:28Z

Also, rebased to fix conflicts.

miri64 · 2019-11-18T13:32:17Z

I made an alternative proposal in #12739. No boards touched, no default BOARDs changed for the applications.

fjmolinas · 2019-11-18T13:36:42Z

periph_uart and stdio_uart are not the same thing... A board that has stdio_rtt still can have a UART...

That is exactly what I'm saying. Out of the 3 boards that use stdio_rtt 2 have periph_uart, therefore they can use stdio_uart. stdio_uart, stdio_rtt and stdio_cdc_acmare three stdio options, they are mutually exclusive but supporting one does not mean the other one can not be supported.

If a test needsethos then the test needs stdio_uart, in which case if possible they should not use stdio_rtt/stdio_cdc_acm as stdio but stdio_uart. stdio_uart it self has a dependency to periph_uart. Out of the three boards the only one that would need to be blacklisted is hamilton which is already the case since it doesn't have periph_uart.

stdio_rtt is not the underlying hardware feature, since stdio is just the mapping of stdio_write and stdio_read. When doing USEMODULE += stdio_rtt you are saying that the default mapping of stdio_write is rtt_write.

miri64 · 2019-11-18T14:01:34Z

If a test needsethos then the test needs stdio_uart, in which case if possible they should not use stdio_rtt/stdio_cdc_acm as stdio but stdio_uart. stdio_uart it self has a dependency to periph_uart. Out of the three boards the only one that would need to be blacklisted is hamilton which is already the case since it doesn't have periph_uart.

It should be noted, that this has also influence on automatic testing. If the board is attached just by the interface the user usually expects (Segger-RTT), if will output nothing via that interface if the test is executed. A separate UART adapter needs to be connected in that case.

stdio_rtt is not the underlying hardware feature, since stdio is just the mapping of stdio_write and stdio_read. When doing USEMODULE += stdio_rtt you are saying that the default mapping of stdio_write is rtt_write.

I know. That is why I think it is reverse logic to say "Don't build this application if stdio_rtt is provided/used/available". The logic should be "Build if stdio_uart is provided/used/available". If 100 boards needs to be adapted for that, so be it, but maybe we can also say somewhere: if there is no other stdio, use stdio_uart which is what this does

RIOT/Makefile.dep

Lines 406 to 408 in 5f4b8ef

    
           ifeq (,$(filter stdio_rtt stdio_cdc_acm,$(USEMODULE))) 
        
             USEMODULE += stdio_uart 
        
           endif

but apparently not central enough.

/cc From #12739 (review)

I'm against this. stdio_uart IMO is not a FEATURE, if we disagree on what a FEATURE is maybe we can have the discussion in #12473.

Then stdio_rtt isn't either.

miri64 · 2019-11-18T14:03:32Z

(just noticed that the new approach doesn't use stdio_rtt as feature anymore, the new title should reflect that).

fjmolinas · 2019-11-18T14:06:51Z

Then stdio_rtt isn't either.

I agree, the issue is that the implementation of stdio_rtt includes the implementation of rtt(rtt_write and rtt_read). I think it would be ideal if both where decoupled and you could have a FEATURES_PROVIDED += jlink_rtt or something like that, but this for me is out of scope of this PR.

fjmolinas · 2019-11-18T14:20:43Z

I know. That is why I think it is reverse logic to say "Don't build this application if stdio_rtt is provided/used/available". The logic should be "Build if stdio_uart is provided/used/available". If 100 boards needs to be adapted for that, so be it, but maybe we can also say somewhere: if there is no other stdio, use stdio_uart which is what this does

I agree it should be a reverse logic but that it should be built if periph_uart is provided, and if it is provided than stdio_uart should be the default stdio for this application.

It should be noted, that this has also influence on automatic testing. If the board is attached just by the interface the user usually expects (Segger-RTT), if will output nothing via that interface if the test is executed. A separate UART adapter needs to be connected in that case.

Many boards need a external device for stdio, but I understand your concern, it's the same issue for boards that use stdio_cdc_acm by default. I'm not sure of the best approach for this, should we maximize the amount of boards for which it is compiled? or the amount of boards that pass without any extra setup?

On an automatic set-up I might rather hook-it up to an FTDI an always use stdio_uart instead of stdio_rtt, if we want to maximize the test coverage for that board. Or I might think that all tests that need extra hardware should be tagged manual for that BOARD and not executed, something in the line of what #11954 wanted to achieve. What do you think?

miri64 · 2019-11-18T14:43:41Z

I think the main issue I have with this approach is that we are replacing one dirty hack with a different dirty hack. I appreciate your work on this, but if the new dirty hack means I have to change my workflow, I rather stick to the old dirty hack.

aabadie · 2019-12-10T10:33:54Z

Rebased now that #12755 is merged.

fjmolinas

There is an issue with the current status because of #9913.

fjmolinas · 2019-12-13T10:58:03Z

boards/hamilton/Makefile.include

-TERMFLAGS = term-rtt
-
-USEMODULE += stdio_rtt
+# Configure Segger's RTT when stdio_rtt is loaded


This is not possible right now since Makefile.dep is included after Makefile.include

You are right, that doesn't work. I came with an another approach which is to set RIOT_TERMINAL to jlink by default with these boards. This way there's no check on the USEMODULE here.
The drawback is that is decouples the automatic use of jlink segger rtt from the fact that stdio_rtt is actually used.

I think this is a none issue, if the user deactivates stdio_rtt by using another stdio_ then they should now they won't get a terminal.

~~Maybe there are other build system related of detecting USEMODULE variables are not changing compared to master ?~~ some of these do support usart, so my comment is not valid.

haukepetersen

just a minor optimization, otherwise looks good to me.

haukepetersen · 2020-01-09T10:33:20Z

boards/ruuvitag/Makefile.include

-TERMFLAGS = term-rtt
+# Configure Segger's RTT when stdio_rtt is loaded
+ifneq (,$(filter stdio_rtt,$(USEMODULE)))
+  TERMPROG = $(RIOTTOOLS)/jlink/jlink.sh


Seems to me this block is duplicated, as its also defined identical for the thingy52. I guess it would make sense to move it to make/tools/serial.inc.mk?!

You are right. I moved the the common code in serial.inc.mk with a slightly different approach.

fjmolinas · 2020-01-10T16:12:59Z

makefiles/tools/serial.inc.mk

+else ifeq ($(RIOT_TERMINAL),jlink)
+  TERMPROG = $(RIOTTOOLS)/jlink/jlink.sh
+  TERMFLAGS = term-rtt


Nice! this should have been here a while ago :)

fjmolinas

One suggestion.

fjmolinas · 2020-01-10T16:18:29Z

boards/thingy52/Makefile.include

-TERMPROG = $(RIOTTOOLS)/jlink/jlink.sh
-TERMFLAGS = term-rtt
+# use JLink Segger RTT by default
+RIOT_TERMINAL ?= jlink


I think this would make sense, it would replicate what is done in Makefile.dep in a way it would work if stdio_ethos, stdio_uart is included.

Suggested change

RIOT_TERMINAL ?= jlink

# HACK: replicate dependency resolution in Makefile.dep

ifeq (,$(filter stdio_%,$(DISABLE_MODULE)$(USEMODULE)))

RIOT_TERMINAL ?= jlink

endif

I don't think so. 'stdio-%' modules used/disabled are known only after makefile.dep has been parsed. So at this stage both variables are probably empty, since makefile.include is parsed before (for the moment). Maybe I'm missing something though.

Unless:

a) passed my the comand line
b) disabled in the application makefile
c) required in the application makefile

And for any of this cases it is required for stdio_something to be included as a module. In our code base it will not work for cases where USEMODULE += stdio_ethos is in an application Makefile.dep. So it is a HACK, and that is why I would have that comment, maybe extend it"

# HACK: replicate dependency resolution in Makefile.dep, only works of `USEMDOULE` or `DEFAULT_MODULE` is set by the command line or in the application Makefile.

It will still make the usage more transparent for most cases. Or maybe include Makefile.dep here and tag is as HACK, for me this would be OK, since we are moving towards including Makefile.include after Makefile.dep in which case this hack could be removed. What do you think?

I applied you proposal for thingy52 and ruuvitag because they can use stdio over UART so that makes sense in this case. For hamilton, since there's no UART configured, I think we can keep the base RIOT_TERMINAL ?= jlink (maybe even without the ?).

I applied you proposal for thingy52 and ruuvitag because they can use stdio over UART so that makes sense in this case. For hamilton, since there's no UART configured, I think we can keep the base RIOT_TERMINAL ?= jlink (maybe even without the ?).

I would keep the ?, but otherwise OK!

Still a bit hacky (IMHO stdio_% should either be a DEFAULT_MODULE for all boards or none... + I believe the stdio_null case is completely ignored here). But my original concerns were addressed.

fjmolinas · 2020-01-13T21:55:17Z

I have no more concern's, it is indeed hacky, but OK. please squash @aabadie

fjmolinas · 2020-01-13T22:00:28Z

Still a bit hacky (IMHO stdio_% should either be a DEFAULT_MODULE for all boards or none... + I believe the stdio_null case is completely ignored here). But my original concerns were addressed.

Can you give a little more context, are you in general against DEFAULT_MODULE or the fact that it is a partial usage. I know you are not blocking because of this, but I'm interested DEFAULT_MODULE is not necessarily the best solution for all cases, it's not very widely used.

For the moment, hamilton, ruuvitag and thingy52 use stdio_rtt by default.

chronos is indirectly blacklisted because of missing periph_uart feature. hamilton doesn't provide periph_uart when stdio_rtt is disabled (because of ethos dependency to stdio_uart) and ruuvitag/thingy52 provide the periph_uart feature so stdio_uart can work on these boards.

pic32 boards now provide an UART, and this the periph_uart feature ruuvitag/thingy52 provide the periph_uart feature so stdio_uart can work on these boards.

ruuvitag/thingy52 provide the periph_uart feature so stdio_uart can work on these boards even if stdio_rtt is disabled because of ethos.

aabadie · 2020-01-14T06:53:42Z

squashed!

aabadie · 2020-01-14T09:28:30Z

@haukepetersen, are you concerns addressed ?

haukepetersen

I really don't like the use of DISABLE_MODULE in the dependency tree regarding the STDIO selection. But I see the problem that it is not too easy to model this in a cleaner way without remodeling large part of the STDIO system.

So even that I don't like it, I am ok with merging this for now. But we should revisit this topic ASAP to model this coherently. Some thoughts that need to be addressed:

STDIO -> tuple of RIOT backend (stdio_uart, stdio_rtt, ...) vs. host tool (pyterm, minicom, jlink.sh)
separation of RIOT backend and multiplexing, e.g. stdio_uart vs ethos
clean definition of default backend and default host tool, with possibility of overriding these
[opt] using multiple STDIO backends in RIOT concurrently? E.g. STDIO over UART (stdio_uart) + STDIO over BLE/CoAP/UDP?!

haukepetersen · 2020-01-15T08:48:35Z

Makefile.dep

@@ -468,6 +468,9 @@ endif

 ifneq (,$(filter stdio_uart,$(USEMODULE)))
  FEATURES_REQUIRED += periph_uart
+
+  # stdio_rtt cannot be used when stdio_uart is loaded
+  DISABLE_MODULE += stdio_rtt


This seems unclean to me: the dependencies should be defined so that only a single stdio_x is used in the first place... Using DISABLE_MODULE like this smells like trouble in the future...

Maybe we should open an Issue to discuss this (something I can do after the release).

haukepetersen · 2020-01-15T09:12:05Z

As we seem to agree and all checks have passed, I take the freedom to merge this PR.

But as said, lets address the STDIO selection thing not too far in the future to make it more intuitive for all boards...

aabadie requested a review from miri64 November 16, 2019 13:48

aabadie force-pushed the pr/stdio_rtt_blacklist branch from 3fb3c84 to b3f4c7d Compare November 16, 2019 16:34

miri64 previously requested changes Nov 18, 2019

View reviewed changes

tests/gnrc_ipv6_ext/Makefile Outdated Show resolved Hide resolved

aabadie force-pushed the pr/stdio_rtt_blacklist branch from c9b6c7e to 5ef1228 Compare November 18, 2019 13:20

miri64 mentioned this pull request Nov 18, 2019

Makefile.dep: make stdio_uart a feature required by ethos #12739

Closed

aabadie force-pushed the pr/stdio_rtt_blacklist branch from 5ef1228 to a5a7c48 Compare November 18, 2019 14:58

aabadie force-pushed the pr/stdio_rtt_blacklist branch from 84392b3 to 16195df Compare December 10, 2019 10:32

fjmolinas reviewed Dec 13, 2019

View reviewed changes

aabadie mentioned this pull request Jan 9, 2020

boards/ruuvitag: fix stdio configuration #13057

Closed

haukepetersen requested changes Jan 9, 2020

View reviewed changes

aabadie force-pushed the pr/stdio_rtt_blacklist branch from 16195df to 8f32159 Compare January 9, 2020 16:38

fjmolinas reviewed Jan 10, 2020

View reviewed changes

aabadie mentioned this pull request Jan 13, 2020

boards: initial import of Adafruit Feather nRF52840 Express #13085

Merged

aabadie force-pushed the pr/stdio_rtt_blacklist branch from 8f32159 to f27c7d5 Compare January 13, 2020 17:21

aabadie mentioned this pull request Jan 13, 2020

boards: add initial support for the PINE64 PineTime smartwatch #12552

Merged

aabadie added 5 commits January 14, 2020 07:48

boards: provide stdio_rtt as default module where required

57d5a1d

For the moment, hamilton, ruuvitag and thingy52 use stdio_rtt by default.

Makefile.dep: disable stdio_rtt when stdio_uart is used

f5cbe00

examples/gnrc_border_router: remove BOARD_BLACKLIST variable

a12af69

pic32 boards now provide an UART, and this the periph_uart feature ruuvitag/thingy52 provide the periph_uart feature so stdio_uart can work on these boards.

examples/suit_update: remove boards from insufficient memory list

024d4ab

ruuvitag/thingy52 provide the periph_uart feature so stdio_uart can work on these boards even if stdio_rtt is disabled because of ethos.

aabadie force-pushed the pr/stdio_rtt_blacklist branch from f27c7d5 to 024d4ab Compare January 14, 2020 06:48

haukepetersen approved these changes Jan 15, 2020

View reviewed changes

haukepetersen merged commit 8b408fc into RIOT-OS:master Jan 15, 2020

aabadie deleted the pr/stdio_rtt_blacklist branch January 15, 2020 09:13

fjmolinas mentioned this pull request Jan 15, 2020

boards/arduino-mkr: feather-m0: sodaq-*: provide stdio over USB and setup automatic flash with bossa #12304

Merged

fjmolinas added this to the Release 2020.01 milestone Jan 15, 2020

chrysn mentioned this pull request Jan 31, 2020

boards: add support for nrf52840-dongle #13146

Merged

8 tasks

fjmolinas mentioned this pull request Apr 2, 2020

*/Makefile.dep: remove usage of DEFAULT_MODULE += stdio% #13791

Merged

make: improve handling of the stdio_rtt module, improve dependency management at application level #12724

make: improve handling of the stdio_rtt module, improve dependency management at application level #12724

Conversation

aabadie commented Nov 16, 2019

Contribution description

Testing procedure

Issues/PRs references

miri64 commented Nov 16, 2019

aabadie commented Nov 16, 2019 • edited Loading

aabadie commented Nov 16, 2019

miri64 commented Nov 17, 2019

aabadie commented Nov 17, 2019

aabadie commented Nov 17, 2019

aabadie commented Nov 18, 2019

fjmolinas commented Nov 18, 2019

aabadie commented Nov 18, 2019

aabadie commented Nov 18, 2019

miri64 commented Nov 18, 2019

aabadie commented Nov 18, 2019

miri64 commented Nov 18, 2019

fjmolinas commented Nov 18, 2019

miri64 commented Nov 18, 2019

miri64 commented Nov 18, 2019

fjmolinas commented Nov 18, 2019

fjmolinas commented Nov 18, 2019

miri64 commented Nov 18, 2019

aabadie commented Dec 10, 2019

fjmolinas 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

haukepetersen 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

fjmolinas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie Jan 10, 2020 • edited Loading

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

fjmolinas commented Jan 13, 2020

fjmolinas commented Jan 13, 2020

aabadie commented Jan 14, 2020

aabadie commented Jan 14, 2020

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Jan 15, 2020

aabadie commented Nov 16, 2019 •

edited

Loading

aabadie Jan 10, 2020 •

edited

Loading