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

Buffer print #589

Merged
merged 9 commits into from
Jun 7, 2024
Merged

Buffer print #589

merged 9 commits into from
Jun 7, 2024

Conversation

MichaelMohn
Copy link
Contributor

@MichaelMohn MichaelMohn commented Aug 26, 2023

TASK PRINT

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
  • Code Build checks pass
  • No merge conflicts
  • Software feature has test associated with it.
  • Software feature has documentation for it updated.
  • If applicable, does software feature have simulation test associated with it (.json & out.json file)
  • Test provides useful information and uses relevant data to accurately represent BPS
    • NOTE: If test file already exists, use that one
  • 2 Members of the team have already reviewed my PR
    • NOTE: Approver will not look at PR if 2 other members haven't reviewed first
  • 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?

Copy link
Collaborator

@tiandahuang tiandahuang left a comment

Choose a reason for hiding this comment

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

Good work 👽. Boilerplate stuff is very well done and styling is pretty good: with a couple minor tweaks for style and cleanup those will be good to go.

Bigger thing is lack of mutual exclusion on the print FIFO -- this needs to get addressed.

Apps/Inc/Print_Queue.h Outdated Show resolved Hide resolved
Apps/Inc/Print_Queue.h Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
@Code-Tomato Code-Tomato self-assigned this Oct 7, 2023
Copy link
Contributor

@connorl309 connorl309 left a comment

Choose a reason for hiding this comment

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

Yall's tests look pretty decent and I don't see any glaring issues with the actual code. some minor fixes/suggestions

Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Drivers/Src/LTC681x.c Outdated Show resolved Hide resolved
Tasks/Inc/Tasks.h Outdated Show resolved Hide resolved
Copy link
Contributor

@ray4477 ray4477 left a comment

Choose a reason for hiding this comment

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

style guide mostly. looks alright for the most part

Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
BSP/STM32F413/Src/retarget.c Outdated Show resolved Hide resolved
Tests/Test_Buffer_Print.c Outdated Show resolved Hide resolved
@@ -86,6 +87,14 @@ void Task_Init(void *p_arg) {
TASK_CLI_STACK_SIZE, // Stack size
);
*/
RTOS_BPS_TaskCreate(&Print_TCB,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a suggestion but theoretically we should only be doing buffer print when actively debugging, so maybe we can only define this task when some debug flag is set. This is optional though and doesn't matter too much if it adds complexity as the task is just above idle task in priority so its prob aight

Apps/Src/Print_Queue.c Outdated Show resolved Hide resolved
Code-Tomato
Code-Tomato previously approved these changes Dec 2, 2023
Copy link

@Code-Tomato Code-Tomato left a comment

Choose a reason for hiding this comment

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

It looks good, and I think it works, I made the block diagram and combed through the bugs.

Print_Queue_Pend(message, &len);

//Port 2 for now
BSP_UART_Write(message, len, UART_USB);

Choose a reason for hiding this comment

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

We should check the speed of this in the future. If it is too slow, we should make it interrupt-based

Print queue completed in theory

builds now hehe

Test

Saturday Changes :)

fix sim issue

build fix

some changes

updated mutex buffer

inshallah it wokrs

work in progress

work in progress

Changed printFifo. Needs >256 char string testing

the message lenght was wrong...

pushin

few logic fixes

comments

making tests

fixed retarget

tests

build

reverted append

fixed retarget again

changed copyright for 2023

applied changes from suggestions

/r

one bug fix

fixed smile?

now it compiles!! *surprised pikachu face

got rid of fifo changes - want a separate PR

comment fix

build

here you go nathan

nathan big brain fix

RENAMED STUFF
@tiandahuang tiandahuang merged commit f2885df into master Jun 7, 2024
3 checks passed
@tiandahuang tiandahuang deleted the bufferPrint branch June 7, 2024 22:01
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