Allows network monitoring (apps repo) using polling and allows the board to provide the IP config at runtime. #1886

davids5 · 2020-09-23T19:18:08Z

Summary

This PR brings in two features and is a sister to apps PR

Low level Net Monitor Polling

Not all boards have an interrupt line from the phy to the Soc. This commit allows the phy to be polled for
link status.

This may not work on all MAC/PHY combination that have mutually exclusive link management and operating
modes. The STM32F7 and LAN8742AI do not have such a limitation.

Support setting the network configuration from the board through BOARDIOC_NETCONF.

This adds the interfaces for the Net Monitor (apps repo) to load the IP addresses and weather or not to use DHCP from the board. This allows the board to save setting on the media of it's choosing and provide the setting to the system in a uniform way.

The modes of support for proto can now be DHCP or static chosen at run time. There is all so support for FALLBACK to use DHCP with fall back static IP after a configurable number of attempts (seconds) see sister apps PR

Impact

None all configurations have Kconfig knobs.

Testing

PX4 FMUV5X

All Kconfig options build and run tested.

include/sys/boardctl.h

Not all boards have an interrupt line from the phy to the Soc. This commit allows the phy to be polled for link status. This may not work on all MAC/PHY combination that have mutually exclusive link management and operating modes. The STM32F7 and LAN8742AI do not have such a limitation.

Support setting the network configuration from the board through BOARDIOC_NETCONF. This adds the interfaces for the Net Monitor to load the IP addresses and wether or not to use DHCP from the board. This allows the board to save setting on the media of it's choosing and provide the setting to the system in a uniform way.

xiaoxiang781216 · 2020-09-25T13:59:16Z

boards/boardctl.c

+
+      case BOARDIOC_NETCONF:
+        {
+          ret = board_get_netconf((struct boardioc_netconf_s *) arg);


xiaoxiang781216 · 2020-09-25T13:59:26Z

include/nuttx/board.h

+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_NETCONF
+int board_get_netconf(struct boardioc_netconf_s *netconf);


This needs to be extended to handle multiple network devices. eth0, eth1, etc. Different network devices will be configured differently. Apparently, this implementation consider only, what?, eth0? This needs to be extended.

@patacongo - it only added to what was already there. But you right, what was there was very limited. My concern would be on an system without a file system, but has a just a small EEPROM. procfs would not be appropriate on such a constrained system.

@patacongo - it only added to what was already there. But you right, what was there was very limited. My concern would be on an system without a file system, but has a just a small EEPROM. procfs would not be appropriate on such a constrained system.

I don't have that concern. I would sacrifice support for tiny systems in order to maintain compatibility with other solutions. That is important. That is the very reason for the existence of the OS and NOT to support tiny hardware.

All of the OS settings in boardioc_netconf_s can be accessed with existing ioctl calls (or using the helpers in netlib). The recommended read-only use of procfs is convenient but optional. The full functionality if present without procfs via ioctl() calls.

xiaoxiang781216 · 2020-09-25T14:24:31Z

include/sys/boardctl.h

+
+struct boardioc_netconf_s
+{
+  enum boardioc_netconf_e flags; /* Configure for static and/or DHCP */


Why need return DHCP/STATIC/FALLBACK from board? it's enough to add FALLBACK Kconfig. Board just need return the saved address.

So it can be set at run time and changed at run time.

So it can be set at run time and changed at run time.

All of the OS settings in boardioc_netconf_s can be managed with existing ioctl calls (or using the helpers in netlib). Those are totally redundant. The recommended read-only use of procfs is convenient but optional. The full functionality if present without procfs via ioctl() calls.

And the flags field. I object strenuously in the that. That MUST not be used. There must NOT be any OS interface of DHCP. DHCP is purely an application, user-space thing and must never contaminate the OS/application interface. This can never come into the OS.

patacongo · 2020-09-25T14:15:22Z

boards/Kconfig

+		Enables support for the BOARDIOC_NETCONF boardctl() command.
+		Architecture specific logic must provide the board_get_netconf()
+		interface.
+


Wouldn't the procfs be a better, more standard way to get configuration information? boardctl() is a necessary kludge, but we should really use it as little as possible since it is inherently non-POSIX, non-standard. procfs is the recommended way.

The preferred way would be to support a procfs folder with files named eth0, eth1, etc. Reading any one of this files would return the configuration of that network.

Is there any precedence for anything like this in Linux?

We should add it. But It is not appropriate for a small constrained system.

We should add it. But It is not appropriate for a small constrained system.

Irrelevant. Let's do what is right for the OS! Commonality of interances, standards, compatibility. THAT is what is important.

patacongo · 2020-09-25T14:18:09Z

include/nuttx/board.h

+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_NETCONF
+int board_get_netconf(struct boardioc_netconf_s *netconf);


This needs to be extended to handle multiple network devices. eth0, eth1, etc. Different network devices will be configured differently. Apparently, this implementation consider only, what?, eth0? This needs to be extended.

patacongo · 2020-09-25T14:25:17Z

include/sys/boardctl.h

+  uint32_t default_router;
+};
+#endif
+


Most of this is already available in procfs/net/. See:

fs/procfs/fs_procfs.c net/procfs/

This is where the NSH ifconfig command gets its network information.

This addition is too redundant and adds no value. In fact, I say that it degrades the OS overall. Let's not merge it.

If you want to extend the procfs with board-specific info then that might be something better to consider. I think the only bit of information not already available is whether the IP address is provided by a DNS server.

Can't we do that is some better way than boardctl()? Can't we do this in the same was as Linux?

I believe if we do this we must not use this kludgey approach. We need to follow the way that Linux does this for compatibility and consistency with some kind of standard:

10.3. Configuring a DHCP Client
To configure a DHCP client manually, modify the /etc/sysconfig/network file to enable networking and the configuration file for each network device in the /etc/sysconfig/network-scripts directory. In this directory, each device should have a configuration file named ifcfg-eth0, where eth0 is the network device name.
The /etc/sysconfig/network-scripts/ifcfg-eth0 file should contain the following lines:

DEVICE=eth0 BOOTPROTO=dhcp ONBOOT=yes

A configuration file is needed for each device to be configured to use DHCP.
Other options for the network script includes:

DHCP_HOSTNAME — Only use this option if the DHCP server requires the client to specify a hostname before receiving an IP address. (The DHCP server daemon in Fedora does not support this feature.) PEERDNS=<answer> , where <answer> is one of the following: yes — Modify /etc/resolv.conf with information from the server. If using DHCP, then yes is the default. no — Do not modify /etc/resolv.conf

https://docs.fedoraproject.org/en-US/Fedora/13/html/Deployment_Guide/s1-dhcp-configuring-client.html

We can add the file IO in the netinit as an option. But what do we do in a system with no file system?

We can add the file IO in the netinit as an option. But what do we do in a system with no file system?

Those systems will be unable to use the polled system with DHCP. Simple.

I will mark this a draft so that it is not merged.

davids5 · 2020-09-25T16:08:51Z

Here are my take aways.

The configuration must follow Linux file location and naming of files and variables.
Boardctl API shall not be used.
The netinit (net monitor) will read the information from files.
It will use IOCTL/net helpers to set the values.
ifconfig needs to be updated to support the thing nuttx can

Usage:
  ifconfig [-a] [-v] [-s] <interface> [[<AF>] <address>]
  [add <address>[/<prefixlen>]]
  [del <address>[/<prefixlen>]]
  [[-]broadcast [<address>]]  [[-]pointopoint [<address>]]
  [netmask <address>]  [dstaddr <address>]  [tunnel <address>]
  [outfill <NN>] [keepalive <NN>]
  [hw <HW> <address>]  [mtu <NN>]
  [[-]trailers]  [[-]arp]  [[-]allmulti]
  [multicast]  [[-]promisc]
  [mem_start <NN>]  [io_addr <NN>]  [irq <NN>]  [media <type>]
  [txqueuelen <NN>]
  [[-]dynamic]
  [up|down] ...

  <HW>=Hardware Type.
  List of possible hardware types:
    loop (Local Loopback) slip (Serial Line IP) cslip (VJ Serial Line IP)
    slip6 (6-bit Serial Line IP) cslip6 (VJ 6-bit Serial Line IP) adaptive (Adaptive Serial Line IP)
    ash (Ash) ether (Ethernet) ax25 (AMPR AX.25)
    netrom (AMPR NET/ROM) rose (AMPR ROSE) tunnel (IPIP Tunnel)
    ppp (Point-to-Point Protocol) hdlc ((Cisco)-HDLC) lapb (LAPB)
    arcnet (ARCnet) dlci (Frame Relay DLCI) frad (Frame Relay Access Device)
    sit (IPv6-in-IPv4) fddi (Fiber Distributed Data Interface) hippi (HIPPI)
    irda (IrLAP) ec (Econet) x25 (generic X.25)
    eui64 (Generic EUI-64)
  <AF>=Address family. Default: inet
  List of possible address families:
    unix (UNIX Domain) inet (DARPA Internet) inet6 (IPv6)
    ax25 (AMPR AX.25) netrom (AMPR NET/ROM) rose (AMPR ROSE)
    ipx (Novell IPX) ddp (Appletalk DDP) ec (Econet)
    ash (Ash) x25 (CCITT X.25)

It would be nice to have a writeable proc fs so that echo "192.168.96.1" > /procfs/eth0/ipaddr

davids5 · 2020-09-25T16:11:20Z

Since this PR is far from what is required, I will close it.

patacongo · 2020-09-25T16:31:54Z

Here are my take aways.

1. The configuration must follow Linux file location and naming of files and variables.

The configuration should follow Linux file location and naming of files and variables unless there is some credible reason to deviate from that.

NuttX has morphed, due primarily to the contributions from Xiaomi. Originally is was a POSIX/Unix compliant OS based on the requirements of OpenGroup.org. But Xiaomi has been pushing harder for Linux compatibility. I have mixed feelings about that. Linux is non-standard, so I am not fully comfortable with using a non-standard OS as our standard (despite the LFS). But it seems that Linux compatible is something that users want more than purely academic POSIX/Unix compliance.

2. Boardctl API shall not be used.

Boardctl API shall not be used unless the functionality is clearly board-related and cannot be realized using standard mechanisms. Boardctl() is not meant to be a dumping ground for quick and dirty implementation when the proper interfaces are available or should be implemented. We want to do things right, not necessarily quickly. That is one of the enemies called out in the INVIOLABLES.

3. The netinit (net monitor) will read the information from files.
   It will use IOCTL/net helpers to set the values.

Certainly, boardctl() commands should not be used to duplicate existing functionality. And certainly nothing related to DHCP should exist at any OS interface.

If there is some other solution for DHCP settings that can be implemented purely in user-space, such as an environment variable, then that might be reasonable too. The file solution should be supported in any event since it is the Linux way and buys us that much more compatibility.

4. ifconfig needs to be updated to support the thing nuttx can

I don't follow. I am not aware of any pressing changes needed to ifconfig.

1. It would be nice to have a writeable proc fs so that echo "192.168.96.1" > /procfs/eth0/ipaddr

procfs is not writable. You can use the IOCTL command to do that (see apps/netutils/netlib). That IOCTL is the standard compatible way to set the IP address on all Linux and BSD systems.

There is no reason why procfs is read-only, but that is the way it is implemented currently. The Linux procfs is writable in some cases, provided that you have privileges.

xiaoxiang781216 · 2020-09-25T17:02:37Z

@davids5 should we keep the first two patches which isn't related to boardctrl?

patacongo · 2020-09-27T19:13:18Z

@davids5 If you need some help implementing this, I can assist you. I am thinking support for the ipcfg file. Let me know.

davids5 · 2020-09-28T09:57:31Z

@patacongo - That would be great. Thank you.

Here is my thinking, from purely a use case perspective, contrasting: Format, Dependency, Flexibility and Recovery

	Current	Configuration file on FS	Configuration in EEPROM
How	Settings are compile time	Setting are in a file or set of files	Setting are in a file or set of files
Format	IP addresses are in long format (not end-user friendly)	human readable	can be longs, with CLI to be human readable
Dependency	None: value are baked in at compile time. Boot timing is not an issues.	File system: To be any better then current solution, must have a writable FS System bring up has to be by init thread, network cannot be initialized unto FS are all mounted	System bring up requires EEPROM driver loaded before network init.
Flexibility	None without rebuilding and deploying	If the FS is not writable it is no better than the current implementation. With a writable FS scales well Defaults could be in ROMFS and overrides could live on SD card.	Configurable. Defaults are in ROM and overrides could live in EEPROM.
Recovery	None , rebuild	If the addresses are wrong, you can not connect, unless you have access to the writable FS. Via (C)connectivity or (P)physical (A)access. (CA, PA, CPA) If the default IP is well know, then removing the SD card would result in a fall back to the default address and the device could be accesses again. Requires PA. Like wise the card could be removed, the IP's updated and replaced. Requires CPA	If the addresses are wrong, you can not connect, unless you have access to the EEPROM or a writable FS. Via (C)connectivity or (P)physical (A)access. (CA, PA, CPA) If the default IP is well know, then removing then disabling the EEPROM, via a button, would result in a fall back to the default address and the device could be accesses again. Requires PA. If the system has a writable FS, i file could be placed there to update the EEPROM Requires CPA

All of these require resources.

Minimizing that impact on an embedded system is important.
Maintaining scalability in NuttX is crucial to it's success. (If NuttX gets too Linux-ised it will be useless on the many SoC's is serves)

Keeping it flexible:
The board level abstraction for getting the network settings, had the appeal of total flexibility of how the settings are managed.

patacongo · 2020-09-28T13:50:58Z

Here is my thinking, from purely a use case perspective, contrasting: Format, Dependency, Flexibility and Recovery
...

The important thing is that NO OS interfaces are involved. DHCPC is an application and must reside wholly in user space.

Minimizing that impact on an embedded system is important.
Maintaining scalability in NuttX is crucial to it's success. (If NuttX gets too Linux-ised it will be useless on the many SoC's is serves)

I don't know who decides what is "too Linux-ised". The important thing is that people must not make up their own operating system interfaces. All operating systems interfaces must be based on standards or at least follow some principles. We follow Linux for two reasons: (1) It provides guidance for how to do many things; How to partition functionality between OS and applications when necessary, What mechanisms to use at the OS interface. It provides good guidance for solving problems. And (2) it makes us compatible with Linux and better able to port Linux code to NuttX.

In this case, the most important principle is that the application must be implemented wholly in user space with no new, non-standard OS interfaces. On that we must be inflexible. The content of files is not so critical. That is not really Linux compatibility; that is compatibility with a common Linux DHCP.client application and I don't there that there are any compatibility concerns. But that usage is still good guidance.

Keeping it flexible:
The board level abstraction for getting the network settings, had the appeal of total flexibility of how the settings are managed.

NO, no, no. No non-standard, kludgey shit OS interfaces. Never! You need to re-read the INVIOLABLES.

DHCP must never have any custom OS interface. It is an application. It is not a part of the OS and must be implemented wholly in user-space.

davids5 · 2020-09-28T15:06:44Z

@patacongo OK Got It.-

What would be the correct structure to support the net information stored in an EEPROM?

patacongo · 2020-09-28T15:33:11Z

@patacongo OK Got It.-

What would be the correct structure to support the net information stored in an EEPROM?

I am working on the solution now for the ascii file case. I will have a draft version of that later today. The interface between the file and the application is a binary interface, converting to ascii before/after write/reads. I think EEPROM might use that binary interface directory with no conversions.

The binary interface is very similar to the binary interface you propose with the boardctl interface.

davids5 · 2020-09-28T15:41:13Z

I am working on the solution now for the ascii file case. I will have a draft version of that later today. The interface between the file and the application is a binary interface, converting to ascii before/after write/reads. I think EEPROM might use that binary interface directory with no conversions.

Perfect.

The binary interface is very similar to the binary interface you propose with the boardctl interface.

This was the app code, it is in c++, it updates the eeprom from the FS.
PX4/PX4-Autopilot@5148fc7

xiaoxiang781216 · 2020-09-28T15:52:20Z

Here is my thinking, from purely a use case perspective, contrasting: Format, Dependency, Flexibility and Recovery
...

The important thing is that NO OS interfaces are involved. DHCPC is an application and must reside wholly in user space.

Minimizing that impact on an embedded system is important.
Maintaining scalability in NuttX is crucial to it's success. (If NuttX gets too Linux-ised it will be useless on the many SoC's is serves)

I don't know who decides what is "too Linux-ised". The important thing is that people must not make up their own operating system interfaces. All operating systems interfaces must be based on standards or at least follow some principles. We follow Linux for two reasons: (1) It provides guidance for how to do many things; How to partition functionality between OS and applications when necessary, What mechanisms to use at the OS interface. It provides good guidance for solving problems. And (2) it makes us compatible with Linux and better able to port Linux code to NuttX.

In this case, the most important principle is that the application must be implemented wholly in user space with no new, non-standard OS interfaces. On that we must be inflexible. The content of files is not so critical. That is not really Linux compatibility; that is compatibility with a common Linux DHCP.client application and I don't there that there are any compatibility concerns. But that usage is still good guidance.

Keeping it flexible:
The board level abstraction for getting the network settings, had the appeal of total flexibility of how the settings are managed.

NO, no, no. No non-standard, kludgey shit OS interfaces. Never! You need to re-read the INVIOLABLES.

DHCP must never have any custom OS interface. It is an application. It is not a part of the OS and must be implemented wholly in user-space.

Agree, it's easy to save/load EEPROM from userspace. We have done the similar(but more complex) thing for WiFi without touching kernel space:
https://github.com/apache/incubator-nuttx-apps/blob/master/wireless/wapi/src/util.c

davids5 · 2020-09-28T15:56:21Z

Here is my thinking, from purely a use case perspective, contrasting: Format, Dependency, Flexibility and Recovery
...

The important thing is that NO OS interfaces are involved. DHCPC is an application and must reside wholly in user space.

Minimizing that impact on an embedded system is important.
Maintaining scalability in NuttX is crucial to it's success. (If NuttX gets too Linux-ised it will be useless on the many SoC's is serves)

I don't know who decides what is "too Linux-ised". The important thing is that people must not make up their own operating system interfaces. All operating systems interfaces must be based on standards or at least follow some principles. We follow Linux for two reasons: (1) It provides guidance for how to do many things; How to partition functionality between OS and applications when necessary, What mechanisms to use at the OS interface. It provides good guidance for solving problems. And (2) it makes us compatible with Linux and better able to port Linux code to NuttX.
In this case, the most important principle is that the application must be implemented wholly in user space with no new, non-standard OS interfaces. On that we must be inflexible. The content of files is not so critical. That is not really Linux compatibility; that is compatibility with a common Linux DHCP.client application and I don't there that there are any compatibility concerns. But that usage is still good guidance.

Keeping it flexible:
The board level abstraction for getting the network settings, had the appeal of total flexibility of how the settings are managed.

NO, no, no. No non-standard, kludgey shit OS interfaces. Never! You need to re-read the INVIOLABLES.
DHCP must never have any custom OS interface. It is an application. It is not a part of the OS and must be implemented wholly in user-space.

Agree, it's easy to save/load EEPROM from userspace. We have done the similar(but more complex) thing for WiFi:
https://github.com/apache/incubator-nuttx-apps/blob/master/wireless/wapi/src/util.c

Yes I saw that. Originally I thought that was the way to go. So I tested just bringing in the JSON parser, but I stooped heading down that road, when it cost 12K just for the parser. All my changes were less that just the parser.

patacongo · 2020-09-28T16:33:00Z

Yes I saw that. Originally I thought that was the way to go. So I tested just bringing in the JSON parser, but I stooped heading down that road, when it cost 12K just for the parser. All my changes were less that just the parser.

The is no size saving or any kind of feature the ever justifies sacrificing any integrity of the OS interface. That is the Holy Grail and the highest of high values of the OS and will never be compromised for any feature.

xiaoxiang781216 · 2020-09-28T17:13:30Z

If JSON is too large, let's try:
https://github.com/apache/incubator-nuttx-apps/tree/master/fsutils/inifile
https://github.com/apache/incubator-nuttx-apps/tree/master/fsutils/inih
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/mtd/configdata.h
Yes, configdata depends on MTD, but it isn't too hard to implement it on top of EEPROM.

patacongo · 2020-09-28T17:22:47Z

If JSON is too large, let's try:
https://github.com/apache/incubator-nuttx-apps/tree/master/fsutils/inifile
https://github.com/apache/incubator-nuttx-apps/tree/master/fsutils/inih
https://github.com/apache/incubator-nuttx/blob/master/include/nuttx/mtd/configdata.h
Yes, configdata depends on MTD, but it isn't too hard to implement it on top of EEPROM.

Please wait a bit. I am completing a draft PR which is kind of a mixture of Linux standard IP configuration file and but with a binary interface compatible with David's original proposal. Let's discuss that that. I will have a draft in place within the hour.

I need to write a test case too before I can complete the draft, but you input before writing the test case would be better.

patacongo · 2020-09-28T17:59:39Z

@xiaoxiang781216 @davids5 Please comment on apache/nuttx-apps#407

patacongo · 2020-09-28T19:55:46Z

A .ini file would be a nice solution as well. You could support multiple network devices in different sections of the .ini file like:

[eth0]
DEVICE=eth0
BOOTPROTO=dhcp

[eth1]
DEVICE=eth1
BOOTPROTO=static
IPADDR=10.0.0.2
NETMASK=255.255.255.0
ROUTER=10.0.0.1

The primarily difference in the way that Linux dhcp.client works is that it uses multiple files like:

File: ipcfg-eth0:

DEVICE=eth0
BOOTPROTO=dhcp

File: ipcfg-eth1:

DEVICE=eth1
BOOTPROTO=static
IPADDR=10.0.0.2
NETMASK=255.255.255.0
ROUTER=10.0.0.1

If there are many network devices then the dhcp.client way is better because it involves re-writing very small files. With only one or two network devices or for a read-only file system, there is no advantage, however.

.ini files are normally read only once by applications at start-up and are essentially read-only. So it is convenient to have that read-only configuration in one rather complex file.

apache/nuttx-apps#407 uses the multiple file, dhcp.client way of doing things.

davids5 requested a review from antmerlino September 23, 2020 19:18

davids5 mentioned this pull request Sep 23, 2020

Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt apache/nuttx-apps#402

Closed

davids5 requested a review from patacongo September 24, 2020 10:41

davids5 force-pushed the master-pr-netconf branch from 8c28ea9 to 8096266 Compare September 24, 2020 10:59

acassis reviewed Sep 24, 2020

View reviewed changes

include/sys/boardctl.h Outdated Show resolved Hide resolved

patacongo removed their request for review September 24, 2020 19:18

davids5 added 3 commits September 25, 2020 06:34

net:Add ARCH_PHY_POLLED option

a332c83

davids5 force-pushed the master-pr-netconf branch from 8096266 to f1640d2 Compare September 25, 2020 13:35

davids5 requested a review from acassis September 25, 2020 13:38

xiaoxiang781216 requested changes Sep 25, 2020

View reviewed changes

xiaoxiang781216 reviewed Sep 25, 2020

View reviewed changes

patacongo reviewed Sep 25, 2020

View reviewed changes

patacongo marked this pull request as draft September 25, 2020 15:07

davids5 closed this Sep 25, 2020

davids5 deleted the master-pr-netconf branch September 25, 2020 16:20

Allows network monitoring (apps repo) using polling and allows the board to provide the IP config at runtime. #1886

Allows network monitoring (apps repo) using polling and allows the board to provide the IP config at runtime. #1886

Conversation

davids5 commented Sep 23, 2020 • edited

Summary

Low level Net Monitor Polling

Support setting the network configuration from the board through BOARDIOC_NETCONF.

Impact

Testing

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

patacongo Sep 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patacongo Sep 25, 2020 • edited

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

patacongo Sep 25, 2020 • edited

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

davids5 commented Sep 25, 2020 • edited

davids5 commented Sep 25, 2020

patacongo commented Sep 25, 2020

xiaoxiang781216 commented Sep 25, 2020

patacongo commented Sep 27, 2020

davids5 commented Sep 28, 2020

patacongo commented Sep 28, 2020

davids5 commented Sep 28, 2020

patacongo commented Sep 28, 2020 • edited

davids5 commented Sep 28, 2020

xiaoxiang781216 commented Sep 28, 2020 • edited

davids5 commented Sep 28, 2020

patacongo commented Sep 28, 2020

xiaoxiang781216 commented Sep 28, 2020

patacongo commented Sep 28, 2020

patacongo commented Sep 28, 2020

patacongo commented Sep 28, 2020 • edited

davids5 commented Sep 23, 2020 •

edited

patacongo Sep 25, 2020 •

edited

patacongo Sep 25, 2020 •

edited

patacongo Sep 25, 2020 •

edited

davids5 commented Sep 25, 2020 •

edited

patacongo commented Sep 28, 2020 •

edited

xiaoxiang781216 commented Sep 28, 2020 •

edited

patacongo commented Sep 28, 2020 •

edited