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

Fwxv 211 write uv cutoff smoke tests #196

Merged
merged 32 commits into from
Jul 26, 2023

Conversation

Taksh-041102
Copy link
Contributor

No description provided.

Copy link
Collaborator

@JarvisWeng JarvisWeng left a comment

Choose a reason for hiding this comment

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

Looks mostly good

@@ -0,0 +1,16 @@
<!--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove all your code not related to this ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't understand what you meant here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like fw103_taksh, hello_world all these files you included shouldn't be included in your PR.
You need to remove them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah alright, noted!

@@ -4,3 +4,5 @@ typedef enum UVCutoffState {
UV_CUTOFF_ACTIVE = 0,
UV_CUTOFF_DISCONNECTED,
} UVCutoffState;

void uv_ligc() {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed a backspace after uv_ligc to resolve an error which scons lint threw.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry what was the error? I don't think uv_ligc is used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed uv_ligc () {} -> uv_ligc() {}.
scons lint said to remove an extra space after uv_ligc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an entirely new line. It shouldn't be here if you're not using it

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should just delete this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had forgot to make this change, it'll be their in the new PR.


if (get_horn_and_lights_horn_state() && !lights_check) {
status = gpio_set_state(&horn, GPIO_STATE_HIGH);
// if(status != STATUS_CODE_OK) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can keep the debug messages or remove the comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

What that means is remove all commented code. If you want to keep them, then uncomment them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, it'll be in the new push.

TASK(smoke_task, TASK_MIN_STACK_SIZE) {
//
//TEST 1 - Disconnected UV status
gpio_set_state(&uv_status,GPIO_STATE_LOW);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also if you could add a LOG_DEBUG before every test just so we know which test is being run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't pushed it yet.

gpio_set_state(&uv_status,GPIO_STATE_LOW);
uv_smoke_logic();
assert( g_tx_struct.uv_cutoff_notification_signal1 == UV_CUTOFF_DISCONNECTED );
LOG_DEBUG("uv_cutoff notification signal set to 'UV_CUTOFF_DISCONNECTED'\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also can you add a 1s delay after ever test? Makes it easier to see in hw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, made this change. Yet to push it.

@@ -0,0 +1,16 @@
<!--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like fw103_taksh, hello_world all these files you included shouldn't be included in your PR.
You need to remove them

@@ -4,3 +4,5 @@ typedef enum UVCutoffState {
UV_CUTOFF_ACTIVE = 0,
UV_CUTOFF_DISCONNECTED,
} UVCutoffState;

void uv_ligc() {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry what was the error? I don't think uv_ligc is used anywhere

#include "uv_cutoff_setters.h"

// TODO(devAdhiraj): update with actual ports and pins
#define UV_CUTOFF_PORT GPIO_PORT_A
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should look at the UV cutoff schematic to find out these pins. If you can't, then just put random ones for now to get your test passing though.


if (get_horn_and_lights_horn_state() && !lights_check) {
status = gpio_set_state(&horn, GPIO_STATE_HIGH);
// if(status != STATUS_CODE_OK) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What that means is remove all commented code. If you want to keep them, then uncomment them


if (get_horn_and_lights_horn_state() && !lights_check) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be keeping this. It's in the original uv_cutoff no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, it is. I see what you mean by treating like an input. noted!

// if(status != STATUS_CODE_OK) {
// LOG_DEBUG("GPIO Horn State could not be set to low\n");
// }
g_rx_struct.horn_and_lights_horn_state = GPIO_STATE_LOW;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be treated like an input. So you should set it in the test, rather than in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I'll make that change.

@@ -163,7 +160,7 @@ int main() {

gpio_init();

gpio_init_pin(&uv_status, GPIO_INPUT_PULL_UP, GPIO_STATE_HIGH);
gpio_init_pin(&uv_status, GPIO_OUTPUT_PUSH_PULL, GPIO_STATE_HIGH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Write a comment here saying that is should be GPIO_INPUT_PULL_UP , but you're faking that for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed :)

Copy link
Collaborator

@JarvisWeng JarvisWeng 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 except that one line

@@ -4,3 +4,5 @@ typedef enum UVCutoffState {
UV_CUTOFF_ACTIVE = 0,
UV_CUTOFF_DISCONNECTED,
} UVCutoffState;

void uv_ligc() {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should just delete this

@JarvisWeng JarvisWeng merged commit d7f479c into main Jul 26, 2023
1 check passed
@JarvisWeng JarvisWeng deleted the FWXV-211-write-uv-cutoff-smoke-tests branch July 26, 2023 17:04
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.

2 participants