Leon's bootcamp is completed, ready for review #258
Conversation
nucleof072rb/Core/Src/main.c
Outdated
| //this is because 0x90 = 0b10010000 | ||
| //where 1 = sgl/diff, 0 = d2, 0 = d1, 1 = d0 | ||
| uint8_t txData[3] = {0x01, 0xD0, 0x00};//send | ||
| uint8_t rxData[3] = {0};//receive |
There was a problem hiding this comment.
txData and rxData do not need to be re-declared every time the ADC channel needs to be read; move these outside the looping section of the main code.
nucleof072rb/Core/Src/main.c
Outdated
| uint16_t Read_ADC_Channel1(void){ | ||
| //this is because 0x90 = 0b10010000 | ||
| //where 1 = sgl/diff, 0 = d2, 0 = d1, 1 = d0 | ||
| uint8_t txData[3] = {0x01, 0xD0, 0x00};//send |
There was a problem hiding this comment.
Try looking at the datasheet again for the config for the second indexed value in txData!
nucleof072rb/Core/Src/main.c
Outdated
| //pull Chip Select to low, as the slave receives (master sends out) data on low edge | ||
| HAL_GPIO_WritePin(GPIOB, GPIO_PIN_8, GPIO_PIN_RESET); | ||
| //now we use spi1 to transmit/receive data by HAL_SPI_TransmitReceive() | ||
| HAL_SPI_TransmitReceive(&hspi1, txData, rxData, 3, HAL_MAX_DELAY); |
There was a problem hiding this comment.
- Ensure you use no magic numbers (i.e. assign every value some variable for code clarity).
- Utilize the error code you receive from the HAL_SPI_TransmitReceive, and determine whether to flag the error or continue with the rest of the code!
nucleof072rb/Core/Src/main.c
Outdated
| //pull Chip Select back to high, as it is supposed to idle in high | ||
| HAL_GPIO_WritePin(GPIOB, GPIO_PIN_8, GPIO_PIN_SET); | ||
| //reconstruct the output into a 10 bit value of type uint16_t | ||
| uint16_t adcOut = ((rxData[1] & 0x03) << 8) | rxData[2]; |
There was a problem hiding this comment.
While this is correct, ensure you don't have redundant declarations every time a section of code is looped! So have the declaration of adcOut outside the main while loop :)
Or tbh, you could just return the calculation instead of needing to allocate memory for adcOut.
nucleof072rb/Core/Src/main.c
Outdated
| uint32_t Convert_to_PWM(uint16_t adcin){ | ||
| //we begin with minimum 1ms, all the way up to 2ms by maxing out adcin as 1023. | ||
| //notice the formula base value + (adcread * delta)/10-bit-max | ||
| return 3000 + (uint32_t)adcin * (6000 - 3000) / 1023; |
There was a problem hiding this comment.
Refer to comment on line 68 on no magic numbers.
nucleof072rb/Core/Src/main.c
Outdated
| MX_SPI1_Init(); | ||
| MX_TIM1_Init(); | ||
| /* USER CODE BEGIN 2 */ | ||
|
|
There was a problem hiding this comment.
You need to start your selected timer's channel (using HAL), otherwise you cannot use it!
EemanAleem
left a comment
EemanAleem
left a comment
There was a problem hiding this comment.
Some minor changes requested. For the transmission bits, you need to use channel 0 for the transmission from the ADC, not channel 1.
nucleof072rb/Core/Src/main.c
Outdated
| //where 1 = sgl/diff, 0 = d2, 0 = d1, 1 = d0 | ||
| uint8_t txData[MCP3004_TRANSFER_BYTES] = { | ||
| MCP3004_START_BIT, | ||
| MCP3004_SINGLE_ENDED|MCP3004_CHANNEL_1, |
There was a problem hiding this comment.
txData[1] should only be MCP3004_SINGLE_ENDED! txData[2] should have the channel used for the ADC which is 0 here. The TIM channel is 1, the one for the ADC is 0.
I would suggest keeping the channel name generic (i.e. MCP3004_CHANNEL) and setting it to 0x00. Then if we're being verbose, txData[2] = MCP3004_CHANNEL.
nucleof072rb/Core/Src/main.c
Outdated
| HAL_StatusTypeDef spiStatus; | ||
|
|
||
| //pull Chip Select to low, as the slave receives (master sends out) data on low edge | ||
| HAL_GPIO_WritePin(GPIOB, GPIO_PIN_8, GPIO_PIN_RESET); |
There was a problem hiding this comment.
Since you've defined variables in lines 39 and 40 for GPIOB and GPIO_PIN_8, use them here in line 94!
3 participants
The previous commits added the following functionalities:
The configuration can build without bugs. Hence we are ready for reviewing.