Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
xiaoxiang781216 merged PR #17007: URL: https://github.com/apache/nuttx/pull/17007 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2379880384 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I ended up implementing changes similar to what @raiden00pl suggested. board defines will be used to initialize smpr1, smpr2, and difsel. chanlist is no longer a structure. Given these changes I think everyone will be satisfied, so I am going to resolve the conversation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2379880384 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I ended up implementing changes similar to what raiden00pl suggested. board defines will be used to initialize smpr1, smpr2, and difsel. chanlist is no longer a structure. Given these changes I think everyone will be satisfied, so I am going to resolve the conversation. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007:
URL: https://github.com/apache/nuttx/pull/17007#discussion_r2355471408
##
arch/arm/src/stm32h5/stm32_adc.c:
##
@@ -624,6 +650,75 @@ static void adc_wdog_enable(struct stm32_dev_s *priv)
adc_putreg(priv, STM32_ADC_IER_OFFSET, regval);
}
+/
+ * Name: adc_wdog1_init
+ *
+ * Description:
+ * Initialize the ADC Watchdog 1 according to Kconfig options.
+ /
+
+static void adc_wdog1_init(struct stm32_dev_s *priv)
+{
+ uint32_t regval;
+ uint16_t low_thresh = 0;
+ uint16_t high_thresh = 4095;
+ uint8_t flt = 0;
+ bool sgl = false;
+ uint8_t chan = 0;
+ bool enable = false;
+
+ /* Initialize Analog Watchdogs if enabled */
+#ifdef CONFIG_STM32H5_ADC1_WDG1
+ if (priv->intf == 1)
Review Comment:
I see what you mean. I will go ahead and fix that. Thanks for the feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
acassis commented on PR #17007: URL: https://github.com/apache/nuttx/pull/17007#issuecomment-3300498476 @kywwilson11 please fix this typo: ``` 1: .codespellrc /home/runner/work/nuttx/nuttx/nuttx/arch/arm/src/stm32h5/stm32_adc.c:555: Peform ==> Perform ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2349804758 ## Documentation/platforms/arm/stm32h5/boards/nucleo-h563zi/index.rst: ## @@ -140,6 +140,17 @@ This configuration configures ADC1_IN3 and ADC1_IN10, which can be accessed at the CN9 A0 and A1 pins respectively. Modify nucleo-h563zi/src/stm32_adc.c to enable more channels. +adc_watchdog: + + +This configuration configures ADC1 channels 3 and 10 as regular, +single-ended channels (ADC1_INP3 and ADC1_INP10). The purpose of this config +is testing analog watchdog 1. Additionally, DMA circular mode is configured +(although not necessary). The watchdog is configured to trigger an interrupt +if the 12-bit adc reading falls outide a window, currently values 0 to 2048 +(0V to V_full_scale/2). The window can be changed through an ioctl or +reconfigured through Kconfig. Review Comment: I updated the documentation as above. Thanks for the feedback. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354516793 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: why not add diff channels support like this: https://github.com/raiden00pl/nuttx/commit/ff7e0f03394e818eade9b9791408b41df61a73e1 easy to use, simple change, no need to modify pins definitions, compatible with other families and most important doesn't change the API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353714144 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: @acassis I agree with you. I understand the need for consistency across the STM32 family. I really do and I am not doing this to be difficult. Before I implement any driver for the STM32H5, I always look at other implementations for consistency sake but also because I do not need to re-do what has already been done. But to accomplish what I need, I thought that the stm32_adc_channel_s structure was the best and cleanest way to pass information I need for the board side to the lower-level driver side. My goal with this pull request was to do 2 things primarily: 1. Add support for differential ADC inputs. 2. Add support for the ADC Watchdogs. I'm adding only code for watchdog1 because the upper level driver does not currently support more than 1 watchdog with the ioctls available. I am remaining steadfast in my position to keep this stm32_adc_channel_s structure. Below is why: 1. The main reason for adding the stm32_adc_channel_s structure was for the differential mode implementation. There is no precedent for this in the stm32 architectures that I found. So nothing to reference. Also, I'm not sure of a better way to pass information that is completely board related. Just like the gpios, which channels are differential is specific to the board implementation, not the architecture. Passing an array of uint8_t types that define the channel is just not enough. How else would I tell the lower driver that a given channel, defined on the board side, is differential. Kconfig is possible but makes less sense than this. 2. The only code actually affected by this structure is code related to the STM32H5. There is currently 1 board supported by the STM32H5, which is the nucleo-h563zi. This pull request updates that board to use this structure. 3. The structure allows me to easily configure sampletime for each channel on a per-channel basis. I find this to be a huge improvement over the implementation in the F7. 4. Other than it being different than what has been done before, I have not seen any valid criticisms of the structure or given potential alternative solutions. It works and does have precedent with the nrf52 at least. Ideally, I do update the stm32f7, stm32h7, and probably other architectures to use this implementation. But as I said before, I am not paid by Apache NuttX. I'm not saying this to be a jerk. I am contributing to NuttX with an application/product in mind for the company that pays me. So in summary, I would like to leave the PR as is with the stm32_adc_channel_s structure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
acassis commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353547243 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: @kywwilson11 everything you said is true and we are grateful for you and your company to adding support for STM32H5. What @raiden00pl is trying to say is: it is important that all STM32 families share similar drivers API, it make the code easier to maintain and a fix that is applied to STM32xx could be moved easily to STM32yy, STM32zz, etc. if you added an improvement that could be included to others STM32 families, it would be nice to add it to there families. But we know sometimes it is a huge task and requires a HW to test it. Imagine the burden of other people adding later improvement to STM32 drivers if each family has a very different driver? We know maybe you don't want to spend some time fixing it now, but not doing it means making the work for other people in the future even harder than the work you will need to have now to improve it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354516793 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: why not add diff channels support like this: https://github.com/raiden00pl/nuttx/commit/181d04cead230f41ac6e4694f28c4ca009f93017 easy to use, simple change, no need to modify pins definitions, compatible with other families and most important doesn't change the API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354516793 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: why not add diff channels support like this: https://github.com/raiden00pl/nuttx/commit/cf36cc8929fb1423a4bd356adb7e351488db47b4 easy to use, simple change, compatible with other families and most important doesn't change the API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354516793 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: why not add diff channels support like this: https://github.com/raiden00pl/nuttx/commit/cf36cc8929fb1423a4bd356adb7e351488db47b4 easy to use, simple change, no need to modify pins definitions, compatible with other families and most important doesn't change the API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007:
URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354480970
##
arch/arm/src/stm32h5/stm32_adc.c:
##
@@ -624,6 +650,75 @@ static void adc_wdog_enable(struct stm32_dev_s *priv)
adc_putreg(priv, STM32_ADC_IER_OFFSET, regval);
}
+/
+ * Name: adc_wdog1_init
+ *
+ * Description:
+ * Initialize the ADC Watchdog 1 according to Kconfig options.
+ /
+
+static void adc_wdog1_init(struct stm32_dev_s *priv)
+{
+ uint32_t regval;
+ uint16_t low_thresh = 0;
+ uint16_t high_thresh = 4095;
+ uint8_t flt = 0;
+ bool sgl = false;
+ uint8_t chan = 0;
+ bool enable = false;
+
+ /* Initialize Analog Watchdogs if enabled */
+#ifdef CONFIG_STM32H5_ADC1_WDG1
+ if (priv->intf == 1)
Review Comment:
this should go to `stm32_dev_s` and be assigned at the compile time. Here is
example how its done for timer and break configuration:
https://github.com/apache/nuttx/blob/34ca49b6f50d92d9a8ed87e40decb6b8a42f712b/arch/arm/src/stm32/stm32_pwm.c#L338-L350
https://github.com/apache/nuttx/blob/34ca49b6f50d92d9a8ed87e40decb6b8a42f712b/arch/arm/src/stm32/stm32_pwm.c#L359-L361
https://github.com/apache/nuttx/blob/34ca49b6f50d92d9a8ed87e40decb6b8a42f712b/arch/arm/src/stm32/stm32_pwm.c#L565-L578
https://github.com/apache/nuttx/blob/34ca49b6f50d92d9a8ed87e40decb6b8a42f712b/arch/arm/src/stm32/stm32_pwm.c#L3196-L3228
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354468408 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: > I evaluated the other implementations in the stm32 family. I did not like any of them for the stm32h5. The stm32f7 could work technically, but honestly I think my implementation is better. The below structures make little sense for the h5. all_same is unnecessary, each channel sample time is independent of the other. To me, this structure did not make a lot of sense for the h5, and the parts that did overly complicated things. But it works, has been here for many years, and doesn't break interface compatibility between ST families. Personal preferences shouldn't be the determining factor here. The ADC in the STM32H5 is nothing special, this IP core is used in other ST chips. I'm not saying your change doesn't make life easier. I'm saying this change brings fewer benefits than the maintenance nightmare it will cause later. > I also needed to find a way to allow for differential inputs. I did not see any STM32 drivers that actually implemented differential mode. Passing the mode in the structure, like the nrf52 does, seemed like a good way for the board level to indicate to the lower level driver that the channels needs to be configured in differential mode. What do you need for diff input? If I remember correctly, just configure GPIO as analog, set STM32_ADC_DIFSEL and set ADCALDIF. This can be solved trivially with two ifdefs and the STM32_ADC_DIFSEL definition in board.h. There is no need to break compatibility between interfaces. > I used the nrf52 as a reference. So this isn't completely out of nowhere. It was suggested to me by a colleague as an example and I thought it worked well for my application. nrf52 is not an STM32 chip. I've been saying from the beginning that what matters is consistency in the ST family, not across all architectures. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2354420487 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: > My goal with this pull request was to do 2 things primarily: Add support for differential ADC inputs. Add support for the ADC Watchdogs. I'm adding only code for watchdog1 because the upper level driver does not currently support more than 1 watchdog with the ioctls available. That's another problem with this PR. One PR and one commit introduces two different, unrelated changes, which makes reviewing them more difficult. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
acassis commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353757798 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I also agree on that @linguini1. I think if everything is settle and @raiden00pl understand your position lets merge it and have some proposal to improve it to other STM32 families (maybe opening an issue to keep track of it). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
linguini1 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353740425 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I think this is a valid criticism of existing NuttX code. If there's no ability in existing drivers to handle differential channel and multiple watchdogs, then either we don't have that feature at all, or there's some new structure added. In general I agree consistency is important, but functionality is too. We can't expect a patch to other families necessarily, although it would be nice. I think if other users need this functionality, they can port it to the other families. Then it's still consistent where this functionality is needed. Does this implementation break anything regarding an actual breaking change? If not, I think Kyle has provided pretty good justification of why this implementation is necessary. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007:
URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353417060
##
arch/arm/src/stm32h5/stm32_adc.h:
##
@@ -498,8 +521,8 @@ extern "C"
struct adc_dev_s;
struct adc_dev_s *stm32h5_adc_initialize(int intf,
- const uint8_t *chanlist,
- int nchannels);
+ struct stm32_adc_channel_s *chanlist
+ , int nchannels);
Review Comment:
I am going to try and drop the hostility. I made the choice about
stm32_adc_channel_s for several reasons. I am going to list them here:
1. I evaluated the other implementations in the stm32 family. I did not like
any of them for the stm32h5. The stm32f7 could work technically, but honestly I
think my implementation is better. The below structures make little sense for
the h5. all_same is unnecessary, each channel sample time is independent of the
other. To me, this structure did not make a lot of sense for the h5, and the
parts that did overly complicated things.
typedef struct adc_channel_s
{
uint8_t channel:5;
/* Sampling time individually for each channel */
uint8_t sample_time:3;
} adc_channel_t;
/* This structure will be used while setting channels to specified by the
* "channel-sample time" pairs' values
*/
struct adc_sample_time_s
{
adc_channel_t *channel;/* Array of channels */
uint8_tchannels_nbr:5; /* Number of channels in array */
bool all_same:1; /* All channels will get the
* same value of the sample time */
uint8_tall_ch_sample_time:3; /* Sample time for all channels */
};
2. I also needed to find a way to allow for differential inputs. I did not
see any STM32 drivers that actually implemented differential mode. Passing the
mode in the structure, like the nrf52 does, seemed like a good way for the
board level to indicate to the lower level driver that the channels needs to be
configured in differential mode.
3. I used the nrf52 as a reference. So this isn't completely out of nowhere.
It was suggested to me by a colleague as an example and I thought it worked
well for my application.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353714144 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: @acassis I agree with you. I understand the need for consistency across the STM32 family. I really do and I am not doing this to be difficult. Before I implement any driver for the STM32H5, I always look at other implementations for consistency sake but also because I do not need to re-do what has already been done. But to accomplish what I need, I thought that the stm32_adc_channel_s structure was the best and cleanest way to pass information I need from the board side to the lower-level driver side. My goal with this pull request was to do 2 things primarily: 1. Add support for differential ADC inputs. 2. Add support for the ADC Watchdogs. I'm adding only code for watchdog1 because the upper level driver does not currently support more than 1 watchdog with the ioctls available. I am remaining steadfast in my position to keep this stm32_adc_channel_s structure. Below is why: 1. The main reason for adding the stm32_adc_channel_s structure was for the differential mode implementation. There is no precedent for this in the stm32 architectures that I found. So nothing to reference. Also, I'm not sure of a better way to pass information that is completely board related. Just like the gpios, which channels are differential is specific to the board implementation, not the architecture. Passing an array of uint8_t types that define the channel is just not enough. How else would I tell the lower driver that a given channel, defined on the board side, is differential. Kconfig is possible but makes less sense than this. 2. The only code actually affected by this structure is code related to the STM32H5. There is currently 1 board supported by the STM32H5, which is the nucleo-h563zi. This pull request updates that board to use this structure. 3. The structure allows me to easily configure sampletime for each channel on a per-channel basis. I find this to be a huge improvement over the implementation in the F7. 4. Other than it being different than what has been done before, I have not seen any valid criticisms of the structure or given potential alternative solutions. It works and does have precedent with the nrf52 at least. Ideally, I do update the stm32f7, stm32h7, and probably other architectures to use this implementation. But as I said before, I am not paid by Apache NuttX. I'm not saying this to be a jerk. I am contributing to NuttX with an application/product in mind for the company that pays me. So in summary, I would like to leave the PR as is with the stm32_adc_channel_s structure. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on PR #17007: URL: https://github.com/apache/nuttx/pull/17007#issuecomment-3300042043 I realized during review that I missed accounting for ADCALDIF when performing calibration for the ADC. I will be updating this PR shortly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007:
URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353417060
##
arch/arm/src/stm32h5/stm32_adc.h:
##
@@ -498,8 +521,8 @@ extern "C"
struct adc_dev_s;
struct adc_dev_s *stm32h5_adc_initialize(int intf,
- const uint8_t *chanlist,
- int nchannels);
+ struct stm32_adc_channel_s *chanlist
+ , int nchannels);
Review Comment:
I am going to try and drop the hostility. I made the choice about
stm32_adc_channel_s for several reasons. I am going to list them here:
1. I evaluated the other implementations in the stm32 family. I did not like
any of them for the stm32h5. The stm32f7 could work technically, but honestly I
think my implementation is better. The below structures make little sense for
the h5. all_same is unnecessary, each channel sample time is independent of the
other. To me, this structure did not make a lot of sense for the h5, and the
parts that did overly complicated things.
typedef struct adc_channel_s
{
uint8_t channel:5;
/* Sampling time individually for each channel */
uint8_t sample_time:3;
} adc_channel_t;
/* This structure will be used while setting channels to specified by the
* "channel-sample time" pairs' values
*/
struct adc_sample_time_s
{
adc_channel_t *channel;/* Array of channels */
uint8_tchannels_nbr:5; /* Number of channels in array */
bool all_same:1; /* All channels will get the
* same value of the sample time */
uint8_tall_ch_sample_time:3; /* Sample time for all channels */
};
2. I also needed to find a way to allow for differential inputs. I did not
see any STM32 drivers that actually implemented differential mode. Passing the
mode in the structure, like the nrf52 does, seemed like a good way for the
board level to indicate to the lower level driver that the channels needs to be
configured in differential mode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353345425 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I did not completely ignore your comment. I made major changes to my commit to conform with NuttX standards. I also removed the p_gpio and n_gpio as you suggested from this structure. But no, I am not going to update the rest of the stm32 chips to use this interface. I am not paid by apache NuttX. I am adding support for a product my company is developing. I don't understand the hostility. Without me, and the work of a former co-worker of mine, there would be 0 support for the STM32H5. @acassis maybe you want to comment. Is this really how moderators are supposed to interact with developers? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
raiden00pl commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2353279180 ## arch/arm/src/stm32h5/stm32_adc.h: ## @@ -498,8 +521,8 @@ extern "C" struct adc_dev_s; struct adc_dev_s *stm32h5_adc_initialize(int intf, - const uint8_t *chanlist, - int nchannels); + struct stm32_adc_channel_s *chanlist + , int nchannels); Review Comment: I see that you completely ignored my comment from the previous PR about this approach. Will you update all other stm32 chips to use this interface? Or will you maintain this file so I can completely ignore it in the future? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
kywwilson11 commented on PR #17007: URL: https://github.com/apache/nuttx/pull/17007#issuecomment-3286762957 I ended up formatting per the below image. Used the procedure from the PR with some minor changes. Going to re-push in a minute. Following this PR, in a few days I will be adding code to connect the watchdog to timers 1 and 8 (for ADC1 and ADC2 respectively). https://github.com/user-attachments/assets/c9117ba4-aa70-482d-88dd-a1cc608ccef8"; /> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
acassis commented on code in PR #17007: URL: https://github.com/apache/nuttx/pull/17007#discussion_r2345166623 ## Documentation/platforms/arm/stm32h5/boards/nucleo-h563zi/index.rst: ## @@ -140,6 +140,17 @@ This configuration configures ADC1_IN3 and ADC1_IN10, which can be accessed at the CN9 A0 and A1 pins respectively. Modify nucleo-h563zi/src/stm32_adc.c to enable more channels. +adc_watchdog: + + +This configuration configures ADC1 channels 3 and 10 as regular, +single-ended channels (ADC1_INP3 and ADC1_INP10). The purpose of this config +is testing analog watchdog 1. Additionally, DMA circular mode is configured +(although not necessary). The watchdog is configured to trigger an interrupt +if the 12-bit adc reading falls outide a window, currently values 0 to 2048 +(0V to V_full_scale/2). The window can be changed through an ioctl or +reconfigured through Kconfig. Review Comment: I think the way you added in the Testing is nice: ``` Procedure Build and flash NuttX with the above Kconfig. Register /dev/adc0 via the board init. Start continuous conversions (DMA circular enabled) and run the adc example to sanity-check normal operation. Tie both CH3 and CH10 to GND → verified no AWD interrupts and ADC continues normally. Tie either CH3 or CH10 to 3.3 V with AWD1 set to “all channels” → ISR fires as expected; conversions continue; AWD1 IRQ is disabled by the ISR. Switch to single-channel AWD1: Select CH3: drive CH3 above the window → ISR fires; drive CH10 above the window → no ISR. Select CH10: mirror the above. Re-enable the watchdog interrupt using the driver IOCTL; confirm subsequent out-of-window events retrigger the ISR. DMA circular mode check: stop conversions, call the DMA reset helper, restart conversions → verify buffer alignment and sample sequence remain correct. ``` ## Documentation/platforms/arm/stm32h5/boards/nucleo-h563zi/index.rst: ## @@ -140,6 +140,17 @@ This configuration configures ADC1_IN3 and ADC1_IN10, which can be accessed at the CN9 A0 and A1 pins respectively. Modify nucleo-h563zi/src/stm32_adc.c to enable more channels. +adc_watchdog: + Review Comment: You can test the Documentation building this way: $ cd nuttx/Documentation/ $ make html Open the generated index.html to see if everything is fine. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
simbit18 commented on PR #17007: URL: https://github.com/apache/nuttx/pull/17007#issuecomment-3284237527 ``` Configuration/Tool: nucleo-h563zi/adc,CONFIG_ARM_TOOLCHAIN_CLANG 2025-09-11 20:12:53 Cleaning... Configuring... Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI Enabling CONFIG_ARM_TOOLCHAIN_CLANG Building NuttX... chip/stm32_adc.c: In function 'adc_setup': Error: chip/stm32_adc.c:1347:15: error: 'struct stm32_dev_s' has no member named 'circular' 1347 | if (priv->circular) | ^~ make[1]: *** [Makefile:170: stm32_adc.o] Error 1 make[1]: Target 'libarch.a' not remade because of errors. make: *** [tools/LibTargets.mk:170: arch/arm/src/libarch.a] Error 2 make: Target 'all' not remade because of errors. /github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory [1/1] Normalize nucleo-h563zi/adc ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] arch/arm/stm32h5: ADC Driver Improvements (Watchdog and Channel Structure) [nuttx]
linguini1 commented on PR #17007: URL: https://github.com/apache/nuttx/pull/17007#issuecomment-3282529891 Thank you for the detailed test log! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
