search

utra-robosoccer / soccer-embedded

Collection of embedded programs for an autonomous humanoid soccer robot
http://utrahumanoid.ca
32 stars 8 forks source link

Add CONTRIBUTING.md with style guide #169

Closed rfairley closed 5 years ago

rfairley commented 5 years ago

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]

Closes: #158

rfairley commented 5 years ago

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 commented 5 years ago

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

rfairley commented 5 years ago

Namespaces - thoughts

Our current namespaces are as follows:

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

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

rfairley commented 5 years ago

Will merge this around 8pm this evening if no objections