Skip to content

Conversation

@IFX-Anusha
Copy link
Contributor

@IFX-Anusha IFX-Anusha commented Jul 23, 2025

By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md
--- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

Implementation of setAnalogwriteFrequency function and documentation
Tests are also implemented

set_fz

Signed-off-by: IFX-Anusha <Anusha.TR@infineon.com>
Signed-off-by: IFX-Anusha <Anusha.TR@infineon.com>
}
return;
} else {
pwm[i].frequency_hz = frequency; // Store the frequency for use in analogwrite
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn´t this storage of the freq in pmw instance attribute be done even if the object is initialized ?

So, no else (block 212-214), and 2 nested "ifs" instead of one in line 206: one for checking if pin is matching, and other for initialzation? If init -> change freq, if no init, we still set the pin freq.

Otherwise if we call setAnalogWriteFrequency(), and the writeAnalog() it will take the default frequency. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was a mistake. What do you think about now, or should it be changed to a nested if?

Now that will store frequency in all cases and if pwm is initialised and pin is matching then there is a change in freq

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, but you only want to change the frequency of the pin passed by argument... not of all pins.
That is why I suggested the 2 ifs... But any other mechanism you can think of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is done

@IFX-Anusha IFX-Anusha requested a review from jaenrig-ifx July 23, 2025 13:57
Signed-off-by: IFX-Anusha <Anusha.TR@infineon.com>
@IFX-Anusha IFX-Anusha force-pushed the set-analog-frequency branch from 1817fd2 to e7eb5b7 Compare July 24, 2025 07:40
}

for (uint8_t i = 0; i < PWM_HOWMANY; i++) {
if (pwm[i].pin == pinNumber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you also check here for both the conditions, like pwm[i].pin == pinNumber || !pwm[i].initialized,
and then assign both the attributes even if the pin was initialized/ not initialized,
pwm[i].pin = pinNumber;
pwm[i].frequency_hz = frequency;

it will simplify the implementation no?
you can then remove the lines from 218 to 225 completely.

Copy link
Contributor Author

@IFX-Anusha IFX-Anusha Aug 4, 2025

Choose a reason for hiding this comment

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

Yes that is possible. But I have a question about this that suppose we call

SetAnalogWriteFrequency( 9, 100) ;

SetAnalogWriteFrequency(8, 50);

Analogwrite( 9, 32676); - Then if this should work then we should have that additional lines 218-225 with an extra check for the pins used.

or else the data will be overwritten

Copy link
Member

Choose a reason for hiding this comment

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

What about implementing a function to allocate the pwm object?
pmw_t * alloc(pin_size_t pinNumber);
The function returns a pointer to the matching pwm[x] object in the array. If that pin is not yet allocated to any of the array objects, it allocates it. Return nullptr if all the positions are allocated.

With that the logic in analogWrite() could be simpler?
In pseudocode:

pmw_t *pwm = alloc(pin);
if pwm is nullptr then return

if not  pwm->initialized then we cyhal_init() and pwm->initialized = true
prepare args for set_duty()
set_duty()
start()

And in setAnalogWriteFrequency() :

pwm_t *pwm = alloc(pin);
if pwm-is nullptr then return

pwm->frequency = freq;
if pwm->initialized then set_duty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is a good approach. Done

Signed-off-by: IFX-Anusha <Anusha.TR@infineon.com>
Copy link
Member

@jaenrig-ifx jaenrig-ifx left a comment

Choose a reason for hiding this comment

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

👯 good!

Copy link
Collaborator

@ramya-subramanyam ramya-subramanyam 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!

@IFX-Anusha IFX-Anusha merged commit 581b3b6 into main Aug 6, 2025
13 checks passed
@IFX-Anusha IFX-Anusha deleted the set-analog-frequency branch August 6, 2025 08:50
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.

4 participants