Add CONTRIBUTING.md with style guide

[rfairley]

This takes from the style guide in our wiki [0], and adds a few things (mostly based on reading the Google C++ style guide). The added parts are marked with PROPOSED throughout this draft.

It turns out this draft of the guide is still quite lengthy, feel free to shorten it where possible or suggest taking out sections which might be too verbose. On the other hand, if there's anything missing, feel free to add it.

Also updated the template with a proposed change of a space between the different groups of includes (see the "Include ordering" section in this doc).

Some sections in Google's style guide that may be applicable to us and worth a read, but I didn't think need to be covered in our guide: [1]

These are the parts that are essentially ripped out of the style guide, I'm thinking we could have these in a tutorial doc in the wiki, on asynchronous programming and I/O.: [2]

• [0] https://github.com/utra-robosoccer/soccer-embedded/wiki/Programming-Style-Guide
• [1] https://gist.github.com/rfairley/e26ed6c2f0f05f92159aa8b668664699
• [2] https://gist.github.com/rfairley/d56f63faaac63cb2e288a3e51d19f23f

Closes: #158

[rfairley]

Requesting review from all - feel free to drop in comments on this over the next week (this is optional). Otherwise we will go over it at the meeting next weekend :)

[rfairley]

Realized #159 needs to be dealt with - going to do some research on this to rethink our current namespaces.

[rfairley]

Namespaces - thoughts

• the main purpose of namespaces is to avoid naming conflicts, but it seems subjective generally how they are used
• some ways people are using them (after looking at StackOverflow)
  • to identify a library of code by grouping reusable code into a namespace
  • to group related classes, functions, etc. together
  • when you find yourself using a common prefix e.g. "NetworkRouter", "NetworkServer" should go under a Network namespace
  • to group different aspects of the codebase, e.g. app/lib/util/etc would each have their own namespace

Our current namespaces are as follows:

• buffer, periph, app, udp_driver, uart, dynamixel, dsp, mocks, os, i2c, udp_interface, lwip_udp_interface, imu, gpio

In my mind (not considering our current directory structure), we have 6 main areas where code has different purposes:

• app: application logic and glue code, like freertos threads, helpers, and instance declarations
• component: configurable building blocks that app uses
• hardware/impl: implementation of hardware interfaces; a hardware implementation must be present for app or component to use the interface
• interface: interfaces that app or component can choose to build on top of (if it wants to be unit tested)
• mock: implementation of mock interfaces; a mock implementation must be present for test to use the interface
• test: unit tests for code in app and component

interface, mock, and hardware

Since interface, mock, and hardware go hand in hand, and correspond directly with some already-known
API (e.g. HAL, LwIP, CMSIS), I like the idea of namespacing these by the API they are wrapping. E.g., hal::UartInterface, hal::GpioInterface, lwip::UdpInterface, cmsis::OsInterface. That way, if say we decided to use the FreeRTOS UDP stack and wanted for an interface for that, it would end up as freertos::UdpInterface. It also keeps us from having to repeat the "Hal", "Lwip" etc prefix in each class name within those namespaces.

Here's what it'd look like:

namespace hal {

class UartInterface { ... };
class UartInterfaceImpl { ... };
class MockUartInterface { ... }; // (UartInterfaceMock would be neat)

}

app and component

Since we are in control of the content in app and components (as in not built to correspond with another library), the namespacing choices are more freeform. I like how we already group things into "suites" of code like dsp, imu, uart etc. which helps indicate what each class's purpose is. For app and component code, I lean towards continuing as we have been. This means the following namespaces:

• buffer
• periph (put peripheral instantiations here)
• app (put freertos threads and some helper code in here)
• udp
• uart
• dynamixel
• dsp
• os
• i2c
• imu
• gpio

An example:

namespace uart {

class UartDriver { ... };
class CircularDmaBuffer { ... };

}

We could add another namespace "comm" or "cmd" to encapsulate the rx/tx helpers in app, and the future protocol buffer components.

If some new class doesn't relate to the other classes in component, we can give it its own new namespace.

I'd only see naming conflicts happening with the classes that are more closely-tied to libraries e.g. the UartDriver and UdpDriver, if e.g. we needed another generic UartDriver based on a library that isn't HAL. However I think that's unlikely, and we can always create sub-namespaces e.g. uart::hal and uart::$other_library.

Also, we should also consider a master namespace for all our code which we don't intend to be a public API e.g. "utra_robot". We don't need this now though so maybe think about it further down the road.

test

Test would continue to have anonymous namespaces.

---

Thoughts?

[tygamvrelis]

Since interface, mock, and hardware go hand in hand, and correspond directly with some already-known API (e.g. HAL, LwIP, CMSIS), I like the idea of namespacing these by the API they are wrapping. E.g., hal::UartInterface, hal::GpioInterface, lwip::UdpInterface, cmsis::OsInterface [...] It also keeps us from having to repeat the "Hal", "Lwip" etc prefix in each class name within those namespaces.

Good point, I like this.

class MockUartInterface { ... }; // (UartInterfaceMock would be neat)

Since the mock is associated with the abstract interface uart::UartInterface (as opposed to a particular implementation, such as hal::UartInterface), the mock should probably reside in a namespace curated for testing.

Also, we should also consider a master namespace for all our code which we don't intend to be a public API e.g. "utra_robot". We don't need this now though so maybe think about it further down the road

Yeah a master namespace is a good idea. We are developing 2 things at the same time: (1) libraries, interfaces, and infrastructure which are (ideally) components that can be reused across various embedded systems projects; and (2) application code for our robot. Perhaps all of our code could be in the namespace utra and our application code (non-reusable logic) could be in utra::soccerbot...

Looking at our current namespace list, I see the following 3 namespaces which seem like they could probably be refactored: udp_driver, udp_interface, lwip_udp_interface. If we have namespaces udp and lwip, then I could see udp::UdpDriver being a class, udp::UdpInterface being an abstract class, and lwip::UdpInterface being a concrete implementation of udp::UdpInterface. Hence, I propose the following rule for the namespaces section of the style guide:

1. Namespaces should be lowercase and convey a single logical idea

(The lowercase part is consistent with what we already have and I don't see any problem with it)

Also, I shared this book in the group chat a while ago (code examples here) and one thing that caught my eye looking at it again just now is the util namespace. The idea of a utility module is just something to keep in mind

[rfairley]

Since the mock is associated with the abstract interface uart::UartInterface (as opposed to a particular implementation, such as hal::UartInterface), the mock should probably reside in a namespace curated for testing.

What I was meaning here, is that uart::UartInterface is really still tied to HAL - it uses HAL types, and the function names and arguments match the HAL functions. If we want to make another mockable UART interface for another library, we'd need to make a new UartInterface for that library. Unless UartInterface.h is intended to be more general?

The example I was concerned about in particular was say if the LwIP Sockets API was used for one component, and the FreeRTOS Sockets API for another. In our current way, we'd have a SocketsInterface.h, containing methods like e.g. socket() but with the chance of some function names colliding from FreeRTOS_socket() and lwip_socket() (they wouldn't collide as the APIs have different function signatures, but we shouldn't rely on that to prevent collision).

To ensure no collision in the above situation, I'd have two files like FreeRrtosSocketsInterface.h and LwipSocketsInterface.h - a separate interface class for each library. Those two classes would be implemented and mocked separately. The classes would look as follows:

class FreeRtosSocketsInterface { ... };
class FreeRtosSocketsInterfaceImpl { ... };
class FreeRtosSocketsInterfaceMock{ ... };

class LwipSocketsInterface { ... };
class LwipSocketsInterfaceImpl { ... };
class LwipSocketsInterfaceMock{ ... };

That's where namespaces would come in to tidy these up:

namespace freertos {

class SocketsInterface { ... };
class SocketsInterfaceImpl { ... };
class SocketsInterfaceMock{ ... };

}

namespace lwip {

class SocketsInterface { ... };
class SocketsInterfaceImpl { ... };
class SocketsInterfaceMock{ ... };

}

Which would eliminate collisions between libraries happening.

Of course we're probably never going to have to use a different API for UART other than HAL, but I still think namespacing UartInterface under hal makes it clear the function signatures in UartInterface came from HAL.

We could do something like with osInterface where it has API functions for more than one library, CMSIS and FreeRTOS (https://github.com/utra-robosoccer/soccer-embedded/blob/master/Common/include/OsInterface.h). I'd be happy with this if we prefixed function names with the API they came from e.g. FreeRTOS_xTaskNotifyWait and CMSIS_osSemaphoreWait. Though, my opinion is that separate interface classes and namespacing would solve that more cleanly in the long term.

[rfairley]

Looking at our current namespace list, I see the following 3 namespaces which seem like they could probably be refactored: udp_driver, udp_interface, lwip_udp_interface. If we have namespaces udp and lwip, then I could see udp::UdpDriver being a class, udp::UdpInterface being an abstract class, and lwip::UdpInterface being a concrete implementation of udp::UdpInterface. Hence, I propose the following rule for the namespaces section of the style guide:

Namespaces should be lowercase and convey a single logical idea

Agreed that these should be refactored, but disagree on how they are refactored for the reasons I mentioned. The way I'd refactor them would be:

udp::UdpDriver // Keep this in udp::, as it is a component we control and is not tied to lwIP
lwip::UdpInterface
lwip::UdpInterfaceImpl
lwip::UdpInterfaceMock

[rfairley]

Yeah a master namespace is a good idea. We are developing 2 things at the same time: (1) libraries, interfaces, and infrastructure which are (ideally) components that can be reused across various embedded systems projects; and (2) application code for our robot. Perhaps all of our code could be in the namespace utra and our application code (non-reusable logic) could be in utra::soccerbot...

Agreed, I like splitting the application code that way.

Also, I shared this book in the group chat a while ago (code examples here) and one thing that caught my eye looking at it again just now is the util namespace. The idea of a utility module is just something to keep in mind

I like the idea of a util file and namespace, that'd help avoid duplicating code. Also interesting to note the comms and protocol namespace setup here: https://github.com/arobenko/embxx/blob/master/embxx/comms/protocol/MsgDataLayer.h#L36.

[rfairley]

Whew, sorry for the slew of text, not meaning to bikeshed too much on this. We can maybe discuss some of these points today's meeting and the next meeting too.

[rfairley]

another proposition from @Shah-Rajab - include a specifier for the type in the variable/function name, this makes it easy when reading/grepping large codebases

[rfairley]

note on namespaces: we will try to automate generation of a library::ApiInterface which we can then automatically generate a library::gmock::MockApiInterface from.

[rfairley]

^ updated after review meeting today - see latest commits. Ready to pull in once approved

[rfairley]

Will merge this around 8pm this evening if no objections