Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs M2 #373

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

Docs M2 #373

wants to merge 54 commits into from

Conversation

Cam0Cow
Copy link
Contributor

@Cam0Cow Cam0Cow commented Dec 2, 2023

Quality Assurance Checklist

To make reviews more efficient, please make sure the software feature meets the following standards and check everything off that meets the quality check. Once everything has been checked, the assigned reviewers will begin the review process. Edit this description to check off the list.

There are exceptions with all guidelines. As long as your decisions are justified, then you are good! Contact the reviewers or the leads about any exceptions.

Requirements

  • Followed Coding Style Guide
  • Presented/discussed in some capacity with others on the Controls team
  • Code Build checks pass
  • No merge conflicts
  • Software feature has documentation for it updated in both ReadTheDocs and the comments
  • Software feature has documentation for it updated
  • Testing
    • Software feature has test associated with it
    • Test provides useful information and uses relevant data to accurately represent Controls
    • Tested on hardware
    • NOTE: If test file already exists, use that one
  • If applicable, have you added the new feature to main.c?
  • Tagged appropriate issue ticket on this PR
  • Did you have fun?

Things to Consider

  • Even if the above are checked, is this the best way of writing my code?
    • It's possible to write code that works, but are there ways to make code more efficient?

@IshDeshpa
Copy link
Contributor

Task linked: CU-868680632 Update Documentation for M2.0

Copy link
Contributor

@NathanielDelgado NathanielDelgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image
Delete the brief and def flags in the docs because they are unnecessary. Sometimes you might want to use brief but not always if there is not much detail to begin with

image
Reorganize header and source file doxygen comments to have this placement and level of detail

Add the copywrite back

Apps/Inc/SendTritium.h Outdated Show resolved Hide resolved
@aidenmadaffri
Copy link

aidenmadaffri commented Jan 10, 2024

Getting an error on the build of the docs:

no theme named 'sphinx_book_theme' found (missing theme.conf?)
make[1]: *** [Makefile:19: html] Error 2
make[1]: Leaving directory '/home/aiden/LHR/Controls/Docs'
make: *** [Makefile:45: docs] Error 2

This was tested on a fresh Ubuntu install and pull of the repo. Install script from Embedded Sharepoint was ran without any apparent errors.

@NathanielDelgado
Copy link
Contributor

Getting an error on the build of the docs:

no theme named 'sphinx_book_theme' found (missing theme.conf?)
make[1]: *** [Makefile:19: html] Error 2
make[1]: Leaving directory '/home/aiden/LHR/Controls/Docs'
make: *** [Makefile:45: docs] Error 2

This was tested on a fresh Ubuntu install and pull of the repo. Install script from Embedded Sharepoint was ran without any apparent errors.

@aidenmadaffri it is telling you you need to install a theme for sphinx to display. This is the theme that is used to generate the HTML documents. You can resolve this using pip install sphinx_book_theme. However, this should be fixed in the install script so maybe make an issue ticket for that? @IshDeshpa

@aidenmadaffri
Copy link

Getting an error on the build of the docs:

no theme named 'sphinx_book_theme' found (missing theme.conf?)
make[1]: *** [Makefile:19: html] Error 2
make[1]: Leaving directory '/home/aiden/LHR/Controls/Docs'
make: *** [Makefile:45: docs] Error 2

This was tested on a fresh Ubuntu install and pull of the repo. Install script from Embedded Sharepoint was ran without any apparent errors.

@aidenmadaffri it is telling you you need to install a theme for sphinx to display. This is the theme that is used to generate the HTML documents. You can resolve this using pip install sphinx_book_theme. However, this should be fixed in the install script so maybe make an issue ticket for that? @IshDeshpa

This resolves the issue, but yes it should be fixed in the install script.

aidenmadaffri
aidenmadaffri previously approved these changes Jan 10, 2024
Copy link

@aidenmadaffri aidenmadaffri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear explanation of all the files and their functions. This will be an excellent resource to reference.

Copy link
Contributor

@KnockbackNemo KnockbackNemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small proofreading suggestions to support all the great work already done.

Apps/Inc/ReadCarCAN.h Outdated Show resolved Hide resolved
Apps/Inc/ReadCarCAN.h Outdated Show resolved Hide resolved
Apps/Inc/ReadCarCAN.h Outdated Show resolved Hide resolved
Apps/Inc/ReadTritium.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCAN.h Outdated Show resolved Hide resolved
@@ -83,11 +85,10 @@ uint8_t BSP_GPIO_Read_Pin(port_t port, uint16_t pinmask){
/**
* @brief Writes data to a specified pin
* @param port The port to write to
* @param pin The pin to write to
* @param pinmask Mask from stm header file that says which pin to write too
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param pinmask Mask from stm header file that says which pin to write too
* @param pinmask Mask from stm header file that says which pin to write to

* @param str : pointer to buffer with data to send.
* @param len : size of buffer
* @param usart : which usart to read from (2 or 3)
* @brief Transmits data to through a specific
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @brief Transmits data to through a specific
* @brief Transmits data through a specific


Overview/Hardware
Overview/Software
To actually build the documentation, run ``make docs``. The resulting ``build/html/index.html`` can then be viewed in any browser. When your pull request is merged into the master branch, the documentation changes should be automatically visible on this website.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're recommending the extension, then we could consider mentioning it here too

Suggested change
To actually build the documentation, run ``make docs``. The resulting ``build/html/index.html`` can then be viewed in any browser. When your pull request is merged into the master branch, the documentation changes should be automatically visible on this website.
To actually build the documentation, run ``make docs``. The resulting ``build/html/index.html`` can then be viewed in any browser or in the editor using this [VSCode extension](https://marketplace.visualstudio.com/items?itemName=ms-vscode.live-server) by navigating to the Controls/Docs/build/html directory. When your pull request is merged into the master branch, the documentation changes should be automatically visible on this website.

* @brief Reads a CAN message from the CAN hardware and returns it to the provided pointers
* @param data where to store the received message
* @param blocking Whether or not this read should be blocking
* @param bus The bus to use, either be CARCAN or MOTORCAN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param bus The bus to use, either be CARCAN or MOTORCAN
* @param bus The bus to use, can either be CARCAN or MOTORCAN

* @param status
* @return true is fail (wrote to an input)
* @return false is success (wrote to an output)
* @param pin the pint to read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param pin the pint to read
* @param pin the pin to read

Copy link
Contributor

@diyarajon diyarajon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Apps/Src/SendCarCAN.c Outdated Show resolved Hide resolved
* @brief Provides the ADC value of the channel at the specified index
* @param hardwareDevice pedal enum that represents the specific device
* @return Raw ADC value without conversion
*/
int16_t BSP_ADC_Get_Value(ADC_t hardwareDevice) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future possible change: renaming it BSP_ADC_Get_Raw_Value. To differentiate between this function and the one that returns Millivoltage.

Apps/Inc/SendTritium.h Outdated Show resolved Hide resolved
@@ -1,11 +1,7 @@
/**
* @copyright Copyright (c) 2018-2023 UT Longhorn Racing Solar
* @file common.h
* @brief
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a brief needed here?

Apps/Inc/fifo.h Outdated
@@ -1,11 +1,7 @@
/**
* @copyright Copyright (c) 2018-2023 UT Longhorn Racing Solar
* @file fifo.h
* @brief
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here?

Apps/Src/SendTritium.c Outdated Show resolved Hide resolved
* @pre The data parameter must be at least 8 bytes or hardfault may occur.
* @param id : pointer to store id of the message that was received.
* @param data : pointer to store data that was received. Must be 8bytes or bigger.
* @return ERROR if nothing was received so ignore id and data that was received. SUCCESS indicates data was received and stored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return ERROR if nothing was received so ignore id and data that was received. SUCCESS indicates data was received and stored.
* @return ERROR if nothing was received, ignore id and data that was received. SUCCESS indicates data was received and stored.

===========
All OS functions return an error code whose value should immediately be checked by a call to ``assertOSError``. The Controls system cannot run without a properly functioning OS, so any OS error is critical and nonrecoverable. Thus, when an OS function returns an error code that is not ``OS_ERR_NONE``, the assertion will fail, and the following steps will occur:
#. If in DEBUG mode, the file and line of the failed assertion will be printed
#. The Array Bypass-Precharge Contactor and Motor Bypass-Precharge Contactor will open
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#. The Array Bypass-Precharge Contactor and Motor Bypass-Precharge Contactor will open
#. The Array Precharge-Bypass Contactor and Motor Precharge-Bypass Contactor will open

The errors asserted by applications include

* :doc:`ReadTritium <ReadTritium>`: Motor controller errors
* Errors sent from the motor controller are asserted by ReadTritium. These are typically nonrecoverable, with the exception of the motor watchdog and hall sensor errors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Errors sent from the motor controller are asserted by ReadTritium. These are typically nonrecoverable, with the exception of the motor watchdog and hall sensor errors.
* Errors sent from the motor controller are asserted by ReadTritium. These are typically nonrecoverable, with the exception of the motor watchdog and hall sensor errors. Motor watchdog and hall sensor errors attempt to recover a set number of times before entering a nonrecoverable state.

* :doc:`ReadTritium <ReadTritium>`: Motor controller errors
* Errors sent from the motor controller are asserted by ReadTritium. These are typically nonrecoverable, with the exception of the motor watchdog and hall sensor errors.
* :doc:`ReadCarCAN <ReadCarCAN>`: Contactor disable conditions
* These errors depend on BPS and include charge disable, contactor open, and BPS trip, as well as missed BPS messages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* These errors depend on BPS and include charge disable, contactor open, and BPS trip, as well as missed BPS messages.
* These errors depend on BPS and include charge disable, contactor open, and BPS trip, as well as missed BPS messages. BPS trip is nonrecoverable, and all the other errors are recoverable. All the errors have callback functions.

IshDeshpa and others added 7 commits January 13, 2024 16:20
* Added clang format/tidy config files, changes to the makefiles to apply them and edited the github workflow to make sure the tools are enforced

* Applied clang format and tidy

* Updated embedded shairpoint which has correct install script
Copy link
Contributor

@diyarajon diyarajon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Apps/Inc/ReadCarCAN.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Apps/Inc/SendCarCan.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants