[PATCH v3] doc: document the pwm command

[Heinrich Schuchardt]

On 05.08.24 14:48, Emil Kronborg wrote:
> On Fri, Aug 02, 2024 at 17:28 GMT, Heinrich Schuchardt wrote:
>> On 01.08.24 12:07, Emil Kronborg wrote:
>>> [...]
>>> +Synopsis
>>> +--------
>>> +
>>> +::
>>> +
>>> +    pwm invert <pwm_dev_num> <channel> <polarity> - invert polarity
>>> +    pwm config <pwm_dev_num> <channel> <period_ns> <duty_ns> - config PWM
>>> +    pwm enable <pwm_dev_num> <channel> - enable PWM output
>>> +    pwm disable <pwm_dev_num> <channel> - disable PWM output
>>
>> To stay consistent with other pages, please, remove the descriptions
>> here. They are available below.
>>
> 
> Hm, all other documented commands in /doc/usage/cmd have a Synopsis
> section except for the write command, which refers to the read command.
> Wouldn't this break the consistency?

I meant, please, remove " - invert polarity", etc.

Best regards

Heinrich

> 
>>> +pwm invert
>>> +----------
>>> +
>>> +The signal's active and inactive states are swapped.
>>
>> This would imply that all invocations irrespective of the polarity value
>> swap the active and inactive state.
>>
>> I guess we could say here:
>>
>> The polarity of the signal is set.
>>
>> If the value of `polarity` is 0, the default polarity is used.
>> If the value of `polarity` is 1, the polarity is inverted.
> 
> Your suggestion is more precise. I will include it in the next revision.
> 
>>> +pwm config
>>> +----------
>>> +
>>> +Configure the period and duty cycle in nanoseconds.
>>
>> According to https://en.wikipedia.org/wiki/Duty_cycle "duty cycle" is
>> the ratio of "pulse active time" and "total period". This implies that
>> it is dimensionless.
>>
>> %s/duty cycle/pulse width/ or "duty period" as in pwm.h.
> 
> That's a good point. I will also include this change.
> 
>>> +
>>> +pwm enable
>>> +----------
>>> +
>>> +Enable output on the configured device and channel.
>>
>> What happens if the device-channel combination is not configured yet?
> 
> The device is enabled with whatever period and duty period are currently
> set. My guess is that the default reset values of the SoC are used.
> 
>      => pwm enable 0 0
>      => echo $?
>      0
>      => pwm config 0 0 20000 14000
>      => pwm enable 0 0
>      => echo $?
>      0
> 
>>> +
>>> +pwm disable
>>> +-----------
>>> +
>>> +Disable output on the configured device and channel.
>>
>>
>>
>> Please, describe the parameters here:
>>
>> pwm_dev_num
>>      Device number of the pulse width modulation device
>>
>> channel
>>      Output channel of the PWM device
>>
>> polarity
>>      * 0 - use normal polarity
>>      * 1 - use inverted polarity
>>
>> duty_ns
>>      pulse width in ns
>>
>> period_ns
>>      cycle time in ns
>>
> 
> The common arguments, namely pwm_dev_num and channel, are already
> described in the Description section. The parameters polarity, duty_ns,
> and period_ns are detailed within the command descriptions. Including
> them again here might be redundant.
> 
> It seems like you want some specific layout. Is there some described
> command I can use as reference? Or is it documented somewhere how to
> describe a command properly? I used some of the commonly used commands
> as reference, but even those are documented differently.
> 
>>> +
>>> +Examples
>>> +--------
>>> +
>>> +Configure device 0 channel 0 to 20 us period and 14 us (that is, 70%) duty cycle::
>>
>> Please, add comma, use 'µs', and don't misuse 'duty cycle':
>>
>> "Configure device 0, channel 0 to 20 µs period time and 14 µs (that is,
>> 70%) pulse width::"
>>
> 
> I wasn't sure if it was allowed to use characters like 'µ', but it's
> good to know that it is allowed; I think it looks better.
> 
> Thanks for reviewing.
>