BSP ADC #70

aaravm4 · 2024-10-05T21:53:04Z

Above is my proposed implementation of ADC Recieve in the BSP. Design decisions were made with the STMF24 in mind.

The PR has an empty BSP_ADC.h file for the actual implementation.

IshDeshpa · 2024-10-07T14:21:12Z

Can you write the header before workday so that I can review it?

IshDeshpa · 2024-10-07T14:22:53Z

Oh also it's not STMF24. We use two different processors on the PCBs:

STM32F413RHT
STM32L431CBT
but then there's different ones on our Nucleo development boards as well that you'll probably use for testing. Your code will generically support most STM32s through the interface of the HAL.

Lakshay983 · 2024-10-07T17:36:45Z

can you check to see if the queue is full (for adding) or empty (for popping) before you do any queue accesses.

IshDeshpa · 2024-10-12T19:36:50Z

put it in Embedded-Sharepoint/bsp/Inc/BSP_ADC.h. The .c file will go in Embedded-Sharepoint/bsp/Src/BSP_ADC.c

…emClock_Config and ErrorHandler

IshDeshpa · 2025-01-08T21:33:04Z

Can you merge main into this branch before I review? seems there are some conflicts.
https://www.youtube.com/watch?v=YINamUvOYhA

IshDeshpa

I'll review the test files next time since compilation is currently failing & there's a good amount of structural changes I had questions about. Let me know if there's any questions about things, and please respond here to the questions/suggestions I've posed so that we keep stuff on the PR. This is a good start, keep up the good work!

IshDeshpa · 2025-01-08T23:56:35Z

bsp/Inc/BSP_ADC.h

+ * Vcc              double
+ * rxQueue          QueueHandle_t   pointer to user-provided Queue handle
+*/
+


See https://en.wikipedia.org/wiki/Javadoc

IshDeshpa · 2025-01-08T23:57:32Z

bsp/Inc/BSP_ADC.h

+*/
+
+
+HAL_StatusTypeDef BSP_ADC_Init(ADC_InitTypeDef init, uint8_t bitNum, double Vcc, QueueHandle_t *rxQueue);


can we make the function names consistent with #76 (adc_init, adc_oneshotread, etc.). Same for file name (i know this conflicts with what I said earlier but just want to keep things consistent)

IshDeshpa · 2025-01-08T23:59:21Z

bsp/Inc/BSP_ADC.h

@@ -0,0 +1,28 @@
+#include "stm32f4xx_hal.h"
+#include "queue.h"


I believe compilation is getting mad because of this being before FreeRTOS.h:
"error: #error "include FreeRTOS.h" must appear in source files before "include queue.h""

IshDeshpa · 2025-01-09T00:05:48Z

bsp/Src/BSP_ADC.c

+
+ADC_HandleTypeDef hadc;
+
+double ADC_RAW_TO_VOLTAGE;  // conversion factor for readings


This conversion can be made a bit more accurate by utilizing fixed point. Instead of storing a double here to represent the decimal, you can store both the VCC and bitNum and do the multiplication and division in the interrupt itself, to preserve some accuracy especially at higher values.
Here's a few resources to make this change/understand why this change needs to be made:
https://stackoverflow.com/a/67234824
https://www.tonmeister.ca/wordpress/2021/07/19/fixed-point-vs-floating-point/

Additionally, some macro magic can probably be used to make defining the bitNum and VCC for each ADC channel a compile time step rather than a runtime step. To sort of understand how this would work, take a look at Nathaniel's PR (https://github.com/lhr-solar/Embedded-Sharepoint/pull/76/files), specifically at how CAN.c and can1_recv_entries.h are used in conjunction. The macro stuff for this wouldn't need to be as complex, but understanding how that works wil give you a good starting point for understanding how to implement something that would work in this context.
This stuff can be a separate PR if you wish (make an issue ticket for it if this is the case). For right now, I'd say changing this to fixed point could be enough.

Finally, this can be static as well. static just means it's a variable that's file-local, not that it necessarily is a constant (const is the keyword used for constants)

IshDeshpa · 2025-01-09T00:06:47Z

bsp/Src/BSP_ADC.c

+
+static QueueHandle_t* adcReadings;
+
+HAL_StatusTypeDef BSP_ADC_Init(ADC_InitTypeDef init, uint8_t bitNum, double Vcc, QueueHandle_t *rxQueue) {


Is it possible for the user to initialize multiple ADC channels/peripherals with different conversion factors (vcc, bitnum) for each? Can this change be made?

IshDeshpa · 2025-01-09T00:17:10Z

bsp/Src/BSP_ADC.c

+        portENTER_Critical();
+
+        // push value to q
+        xQueueSendFromISR(adcReadings, &realVal, &higherPriorityTaskWoken);
+
+        // give semphr
+        xSemaphoreGive(adc_send_semaphore);
+
+        portEXIT_Critical();


This doesn't seem like it's a critical section. The QueueSendFromISR already internally enters & exits a critical section, which means these portENTER_Critical/portEXIT_Critical calls aren't required (xQueueSendFromISR is already threadsafe). Furthermore, as explained elsewhere, I don't think this semaphore around the queue send is required either

IshDeshpa · 2025-01-09T00:20:04Z

bsp/Src/BSP_ADC.c

+    /**
+     * GPIO Inits for ADC Reading [using ADC1]
+     * PA_3     ADC1_3
+     * PC_0     ADC1_10
+     * PC_3     ADC1_13
+     * PA_7     ADC1_7
+     * PA_4     ADC1_4
+     * PB_1     ADC1_9
+     * PC_2     ADC1_12
+     * PA_0     ADC1_0
+     * PB_0     ADC1_8
+     * PA_5     ADC1_5
+     * PA_6     ADC1_6
+     * PA_7     ADC1_7
+     */


Is this always true? How would we make these pin definitions hardware agnostic in the future? (hint hint, I suggest some sort of macro thing). This doesn't need to be handled in this PR per-say but I would like you to start thinking about it for the future PRs.

IshDeshpa · 2025-01-09T00:22:03Z

bsp/Src/BSP_ADC.c

+}
+
+
+void HAL_ADC_MspDeInit(ADC_HandleTypeDef *hadc) {


could/should we add an adc_deinit?

IshDeshpa · 2025-01-09T00:31:56Z

stm/stm32f4xx/stm32f4xx_hal_init.c

+void Error_Handler(void)
 {
  __disable_irq();
  while (1)


Perhaps this should be kept static and file-local, and a completely different error_handler could be used for ADC (could call it adc_error_handler and keep it file-local to ADC as well). See my comments on the need for the ADC error-handler as well

IshDeshpa · 2025-01-09T00:33:57Z

bsp/Inc/BSP_ADC.h

+#include "queue.h"
+#include "FreeRTOS.h"
+#include "stm32xx_hal.h"
+#include "stm32f4xx_hal_init.c" // this can be put in "stm32xx_hal.h" later


we should not be including .c files

added adc header

1fbf815

aaravm4 requested review from IshDeshpa, Lakshay983, NathanielDelgado and ppatra126 October 5, 2024 21:53

IshDeshpa marked this pull request as draft October 5, 2024 23:30

IshDeshpa assigned Lakshay983, NathanielDelgado, connorl309, ppatra126 and aaravm4 and unassigned Lakshay983, NathanielDelgado, connorl309 and ppatra126 Oct 7, 2024

IshDeshpa changed the title ~~BSP ADC Stuff --- Aarav~~ BSP ADC Oct 7, 2024

IshDeshpa requested a review from connorl309 October 7, 2024 14:07

IshDeshpa linked an issue Oct 7, 2024 that may be closed by this pull request

BSP_ADC #8

Open

completed header file for adc

ea4220e

aaravm4 added 7 commits October 12, 2024 15:35

Merge branch 'main' into draft

32b9e5b

reorganized BSP files

c88b886

switched includes to just stm32xx.h

eaa09d5

flashed to nucleo, debugging time

b305092

made tests for poll and interrupt, boilerplate code for actual BSP

f5e5327

added imports for adc and changed gpio settings

d71dc04

code before using auto-generated cube

cf7313b

aaravm4 added 2 commits October 26, 2024 12:08

ADC Poll Test Working; Changed STM32 HAL Init Headers to correct Syst…

30c94f7

…emClock_Config and ErrorHandler

saving cuz my computer is cooked

873d8ac

IshDeshpa marked this pull request as ready for review January 3, 2025 23:12

final commit before pr

4ec1182

resolved merge conflict, merging with main

8d06439

IshDeshpa requested changes Jan 9, 2025

View reviewed changes

IshDeshpa requested review from Shaun345 and removed request for connorl309 January 9, 2025 00:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BSP ADC #70

BSP ADC #70

aaravm4 commented Oct 5, 2024

IshDeshpa commented Oct 7, 2024

IshDeshpa commented Oct 7, 2024 •

edited

Loading

Lakshay983 commented Oct 7, 2024

IshDeshpa commented Oct 12, 2024

IshDeshpa commented Jan 8, 2025

IshDeshpa left a comment

IshDeshpa Jan 8, 2025

IshDeshpa Jan 8, 2025 •

edited

Loading

IshDeshpa Jan 8, 2025

IshDeshpa Jan 9, 2025 •

edited

Loading

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

IshDeshpa Jan 9, 2025

		*/


		HAL_StatusTypeDef BSP_ADC_Init(ADC_InitTypeDef init, uint8_t bitNum, double Vcc, QueueHandle_t *rxQueue);

		@@ -0,0 +1,28 @@
		#include "stm32f4xx_hal.h"
		#include "queue.h"


		ADC_HandleTypeDef hadc;

		double ADC_RAW_TO_VOLTAGE; // conversion factor for readings


		static QueueHandle_t* adcReadings;

		HAL_StatusTypeDef BSP_ADC_Init(ADC_InitTypeDef init, uint8_t bitNum, double Vcc, QueueHandle_t *rxQueue) {

BSP ADC #70

Are you sure you want to change the base?

BSP ADC #70

Conversation

aaravm4 commented Oct 5, 2024

IshDeshpa commented Oct 7, 2024

IshDeshpa commented Oct 7, 2024 • edited Loading

Lakshay983 commented Oct 7, 2024

IshDeshpa commented Oct 12, 2024

IshDeshpa commented Jan 8, 2025

IshDeshpa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IshDeshpa Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IshDeshpa Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IshDeshpa commented Oct 7, 2024 •

edited

Loading

IshDeshpa Jan 8, 2025 •

edited

Loading

IshDeshpa Jan 9, 2025 •

edited

Loading