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

Pieter patch 1 (OneShot mode working) #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PieterBosElectro
Copy link

OneShot mode function:
Takes 1 converstion and waits till trigger of timer and takes another converstion

User defines in .h file:

//user commands
#define readConversionData		0b01000011; // Device address 1, incremental read starting at register address 0 (ADC Data register) 0x43
#define fullResetFastCmd		0b01111000; // Fast command to reset device 0x78
#define startConversionFastCmd  0b01101000; // Fast command to start a conversion 0x68

@PieterBosElectro PieterBosElectro changed the title Pieter patch 1 Pieter patch 1 (OneShot mode working) Jan 30, 2023
@mnemocron mnemocron self-requested a review January 30, 2023 14:26
Copy link
Member

@mnemocron mnemocron left a comment

Choose a reason for hiding this comment

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

I don't think the code like this runs or even compiles.
But it has good work in it that can be adapted.

//Use your own debug mode
//Lcd_cursor(&lcd,0,9);
//Lcd_string (&lcd,"ADC POR");
MCP3561_Init_ch0(&hspi1);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where MCP3561_Init_ch0 was defined.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, i have different init for different channels.

@@ -413,6 +413,61 @@ void HAL_GPIO_EXTI_Callback(uint16_t GPIO_Pin)
}
}
}

/*@brief this fuction does One-Shot for MCP3564 */
void ADC_One_Shot(SPI_HandleTypeDef *hspi)
Copy link
Member

Choose a reason for hiding this comment

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

ADC_One_Shot is not called anywhere, but much more importantly, it is not defined before.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think the code like this runs or even compiles. But it has good work in it that can be adapted.

I compiles on my machine. But thais not a good reason ;)

if ((status & 0x01) == 0x00)
{
//Use your own debug mode
//Lcd_cursor(&lcd,0,9);
Copy link
Member

Choose a reason for hiding this comment

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

Remove code fragments regarding LCD

Copy link
Author

Choose a reason for hiding this comment

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

Yo, thanks! My first code review. So thanks for the opportunity. And of course thanks for the debugging work 😃 As you can see, I can't just merge it but incorporate your input a little differently later.

Thanks! i will implement you suggestions

@@ -95,6 +95,64 @@ void MCP3561_Init(SPI_HandleTypeDef *hspi){


}
void MCP3561_Init_One_Shot(SPI_HandleTypeDef *hspi){
Copy link
Member

Choose a reason for hiding this comment

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

Define this function in the .h file too

Copy link
Author

Choose a reason for hiding this comment

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

I will do


// be careful with the bitwise or operator "|"
cmd[0] = MCP3561_CONFIG0_WRITE;
cmd[1] = MCP3561_CONFIG0_CLK_SEL_EXT; // clock selection
Copy link
Member

Choose a reason for hiding this comment

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

Building the cmd bytes like this is outdated. Use the mcp3564_conf.h method.


cmd[0] = MCP3561_MUX_WRITE;
cmd[1] = (MCP3561_MUX_CH_AGND << 4) | MCP3561_MUX_CH0;
//cmd[1] = (MCP3561_MUX_CH_AGND << 4) | MCP3561_MUX_CH0; //Deze werkt
Copy link
Member

Choose a reason for hiding this comment

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

Love a good Dutch comment:)

// @see Datasheet Table 5-14 on p. 54

/*
cmd[0] = MCP3561_SCAN_WRITE;
Copy link
Member

Choose a reason for hiding this comment

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

remove this, I guess

@mnemocron
Copy link
Member

Yo, thanks! My first code review. So thanks for the opportunity.
And of course thanks for the debugging work 😃
As you can see, I can't just merge it but incorporate your input a little differently later.

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