Maintenance Tracker

[tygamvrelis]

Purpose

This issue is where we can track all our ideas for what we want to improve after Dec. 1. They can be anything, from tiny things that irk you but you don't quite know how to solve yet, to big things you think totally flew over our heads.

Items

• Change all FreeRTOS calls, constants, etc. to the CMSIS equivalents. This also means we should change the OsInterface class to use the CMSIS functions
  • Reason: It is more portable as CMSIS is a standard API. They also have some pretty useful APIs, my favourite of which right now is the ones for condition variables (osSignalWait, osSignalSet, etc., implemented via task notifications) which I think we'd get a lot of use out of
• Define peripheral handles in some unified location (e.g. instead of using &huart5 in calls to PC interface functions, we would instead use something like PC_USART_HANDLE (Maintenance: Define peripheral handles in some unified location #141)
  • Reason: This will it easier to deploy our code onto the different boards we support (F4, F7)
• Decide on file name and namespace conventions (Style guide purge + finalize style conventions+ CONTRIBUTING.md document #158)
  • Reason: Better to have this style be more consistent
• Decide on a few key namespaces to group things by (Rename namespaces and other identifiers in the codebase #159)
  • Reason: As we grow our code base, we will have new peripherals, data structures, algorithms, etc. We should agree on namespaces these things should go in, otherwise things will get hard to maintain fast
• Consider making THREADED a compiler option (-D) as opposed to a macro defined in a common header (will be done in SystemConf should be decoupled from Common/component #177)
  • Reason: Some tests may inadvertently include the system header through some dependency chain, but they require a different value for THREADED (e.g. they require it to be defined but it is not, or vice versa)
• Integrate a continuous integration (CI) service for automated builds and unit testing (for pull requests)
  • Reason: Better development flow
• Set up some sort of server that we can log into if we want to test our code at runtime
  • Reason: This is long overdue. It's very important that it's easy for people to test their code at runtime as this is always the final check and it is inconvenient for everyone to have to do it in person (additionally there can be a lot of competition if everyone tries to do that during our weekly meetings)
• Add a CI linter (code style checker)
  • Reason: Improve code consistency, and can have checks that make sure malloc and new are not used (can be missed in code reviews)
• Consider separating threads into different files
  • Reason: Hopefully less merge hell if high-level application code is managed in separate files as opposed to just freertos.cpp
• Remove names after @file in Doxygen (Doxygen: audit codebase to make documentation consistent #161)
  • Reason: Apparently we do not actually need any name after this tag, so we should just remove it using a script
  • Done in Update Udp identifiers, rename hardware and mock filenames/classes, remove filename from @file and add script for it #194
• Consider separate TX and RX threads for PC program
  • Reason: It just seems natural to consider these as separate execution contexts, and maybe it could open up the door for some cool opportunities. Should explore it a bit

[tygamvrelis]

• Consider using a watchdog timer (WDT) to recover from critical faults
  • Reason: if we have an unrecoverable system fault, we need to be able to recover. Well, it would be a good learning experience to design it like this anyway
• Investigate low-power modes (and how to do this with FreeRTOS)
  • Reason: we know our system is busy, but it does (as of this writing) spent time idle. We should see if we can enter a low-power state to save power consumption
• Create permanent logs
  • Reason: it would be great if we could log events (particularly important ones) in a non-volatile memory. We may need support from the electrical team to get an EEPROM on the PCB (probably would use SPI)

[rfairley]

Define peripheral handles in some unified location

+1

[rfairley]

• check convention of <> for library file includes and "" for project file includes is applied consistently throughout project

[rfairley]

• way to ensure CubeMX settings are synced across boards
  • e.g. common components such as FreeRTOS should have equivalent settings whether on F4 or F7. if Cube settings change on one board, they should also change on the other

[tygamvrelis]

• Make sure all private members are using the prefix m_ (done in Rename identifiers #193)
  • Reason: Conforms better to our style guide

[tygamvrelis]

• Allow different IO type to be used for TX and RX in UartDriver
  • Reason: No reason why not. The IO type does not need to be the same for both, it really depends on the requirements of the application

[rfairley]

• Check if volatile needs to be used in some places throughout application code (mentioned by @tygamvrelis )
  • Reason: if we do a release build with optimizations, compiling with O3 might subtly change the critical parts of the code. We should verify that volatile has been used with this in consideration.
  • Once we do this, make sure to add a note in CONTRIBUTING.md on when to use volatile, and make note of the compiler optimization level.

[rfairley]

• make sure type sizes are explicitly declared (e.g. uint32_t instead of int). Applies to enumerations and any other higher-level types that have an underlying numeric type as well.
  • Reason: best practice when compiling embedded code. Mostly we have been good about doing this, but this may have been missed for the less-obvious types.

[rfairley]

• check for unnecessary #includes and remove. Edit: after PeripheralInstances: fix missing SystemConf.h include #167, we should go through and make sure includes have been done in the direct source/header file where the symbols have been used.
  • Reason: taking things out of source files and refactoring may have left some includes that are no longer used. They shouldn't cause a problem but it is unneeded preprocessor logic.
  • Maybe it's better to go for a tool for this https://stackoverflow.com/questions/614794/detecting-superfluous-includes-in-c-c.

[rfairley]

• Split out testable components such as UartInterface, OsInterface, UdpInterface from the project-specific Common/includes and Common/hardware folders. This involves moving the source files somewhere that they can be easily included by projects that are not necessarily related to Robot. It might look like a repository imported as a submodule like utra-robosoccer/soccer-testable-hardware or utra-robosoccer/soccer-hardware-mocks.
  • Reason: Projects like omnibot may benefit from this if we want to quickly add unit tests. We should be able to move around project-agnostic files easily when starting a new project, e.g. only one folder of includes needs importing to get the full suite of testable interfaces.
  • This goes hand in hand with rethinking the namespaces. Components should be imported individually, e.g. like how gtest has the client import ::testing::Return. Though, this work to make the name spaces fit for "public" projects need not be done as part of the item Decide on a few key namespaces to group things by. This task can happen further down the road.

[rfairley]

• A standard way to patch the generated files by CubeMX. Might take the form of a .patch file that we automatically apply somehow to the vanilla file while building. We can find out how much files have deviated from generated files already by running git diff --patch -- <path to vanilla generated file> <path to edited generated file> (add --no-index before the -- to use for files that aren't in a git repository, https://git-scm.com/docs/git-diff).
  • Reason: It is still beneficial to quickly go through the flow of 1) change a parameter or add something in CubeMX, 2) regenerate the file, and 3) apply edits from before we may have manually made to the file. Comes up in Maintenance: Define peripheral handles in some unified location #141 when usart.c needs to be modified with custom edits.

[tygamvrelis]

• Style purge—revise the programming style guide to make it much shorter. Make the focus on writing code that has a consistent look and feel, then purge all our code to follow these look and feel rules (maybe via an automated tool...?) (Style guide purge + finalize style conventions+ CONTRIBUTING.md document #158)
  • We decided on conventions with Add CONTRIBUTING.md with style guide #169 merged. Need to do the work of purging each file now.

[rfairley]

• Ensure doxygen comments are made at the function declarations in header files, not source file definitions.
• Reason: should be consistent; there are some files where I missed this.

[rfairley]

• Better automate loading the template into a new source/header file in System Workbench.
  • Populate new source files automatically with the template
  • Automatically generate fields such as include guards which might follow a convention corresponding to the path from the toplevel directory.
  • Reason make the template more visible to developers and avoid time manually copy/pasting/checking

[rfairley]

• Automatic indentation for eclipse project
• Reason: more easily format code to our chosen indentation. Eclipse already has a builtin Allman style formatter, which we can adapt if needed

[rfairley]

• Update include paths in .cproject file after unified Robot changes (Gdharan combine f4 f7 #171)
  • Reason: not needed for building as we are now using the makefile, but we should make sure these are synced so the IDE can index the code properly

[rfairley]

• (noting from Add CONTRIBUTING.md with style guide #169): Try out using doxygen TODOs in comments, and decide if we want to mandate using the Doxygen style TODO tag in addtion to the traditional TODO:
  • Can start from https://stackoverflow.com/questions/8535635/can-doxygen-easily-be-configured-to-recognise-todo-and-fixme-lines and http://www.doxygen.nl/manual/commands.html#cmdtodo
  • Need to update the CONTRIBUTING.md doc if we decide to use \todo.

[rfairley]

• Think about where test fixtures can be better utilized in unit tests.
  • This applies to UdpDriver_test.cpp and UartDriver_test.cpp really. The tests are correct in declaring new mock instances for each test and a new Udp/UartDriver per test. We can probably optimize and have fewer declarations overall though, by declaring common base Udp/UartDriver options in a fixture.