potential dc period initializing order issue when pwmX exist

[mj2068]

rpi_hardware_pwm/rpi_hardware_pwm/__init__.py

Lines 54 to 67 in eea3028

	self._duty_cycle = 0
	self._hz = hz
	if not self.is_overlay_loaded():
	raise HardwarePWMException(
	"Need to add 'dtoverlay=pwm-2chan' to /boot/config.txt and reboot"
	)
	if not self.is_export_writable():
	raise HardwarePWMException(f"Need write access to files in '{self.chippath}'")
	if not self.does_pwmX_exists():
	self.create_pwmX()
	self.__init_frequency(self._hz) # set this first before DC
	self.__init_dc(self._duty_cycle)

hi, here's something that i've found, not sure how of an edge case this counts but, if pwmX folder exists and duty_cycle also has a value(could happen if stop() wasn't called previously), then __init_frequency(...), __init_dc(...) sequence could fail for certain init hz. Because __init_frequency(...) runs first, it calculates a period nanoseconds using hz and writes to period, this could trigger one of the "Invalid argument" conditions, if the period is greater than that existing duty cycle.

i tested it, verified what i just described, and i think the order should be first __init_dc to 0, then __init_frequency, but the author specifically commented it should __init_dc first, so that throws me off... maybe something i don't see.

if you could confirm this is valid i can pr and fix it if needed.

thx.

[mj2068]

okay... it didn't take long for me to know why the author commented that.

it's because for a freshly created pwmX folder, the three files enable, period and duty_cycle all store 0, then writing 0(actually any value) to duty_cycle when period is 0 will trigger yet another "Invalid argument".

so in order to cover both cases, new pwmX folder or previously existed, we kinda have to check duty_cycle value:

with open(os.path.join(self.pwm_dir, "duty_cycle"), "r") as f:
  exist_dc = int(f.read())
  if exist_dc != 0:
    self.__init_dc(self._duty_cycle)
self.__init_frequency(self._hz) # set this first before DC 

ps: a nonexhaustive list of "Invalid argument" writes to the respective files:

enable

• 0 or 1 when period is 0

duty_cycle

• any value when period is 0
• greater than period

period

• less than 15
• greater than ~4.29e10(about 43 seconds, period upper limit)
• less than duty_cycle

[CamDavidsonPilon]

Thanks for investigating further @mj2068. I haven't sat down to test yet, but what about if we "blow away" any existing pwmX dirs at init to always start with a clean system?

[mj2068]

but from what i understand the sysfs mechanism is that we can't manipulate the pwmX dir directly, we can only write to the files export and unexport. i've tried, can't even sudo rm -rf pwm0 as root, "rm: cannot remove '...': Operation not permitted".

so if we can't delete the pwmX dir, what if we unexport and export again. i've tested this, the states in enable, period and duty_cycle all retain values prior to unexport.

and one further somewhat surprising behavior is, say, we're in a state where file values are:

• period: 10000
• duty_cycle: 5000
• enable: 1

so 50% enabled working duty output signal. and we unexport this channel, pwmX dir disappears, the 50% duty signal will stop on that pin, whatever your load is will turn off. then if we export it again, the pwmX dir shows up, the values in the files will not have changed, but the pin signal doesn't turn on automatically. right now it's actually in a desynchronized state, enable says 1 but no signal. write 1 to enable does nothing, write to it 0 and then 1 again finally turn on the original 50% duty again.

i don't know if this is generally true among different kernel versions, but my environment is pi 4 with Linux version 5.15.0-1087-raspi.

ps: i discovered all these when trying to implement "read only" support when constructing an HardwarePWM instance, since right now it always initializes.

and sorry for the long reply, i didn't know how to condense...