Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linuxfs-pwm provider fixes for Pi5 operation. Default now pwmchip2, w… #339

Merged
merged 2 commits into from
Apr 8, 2024
Merged

linuxfs-pwm provider fixes for Pi5 operation. Default now pwmchip2, w… #339

merged 2 commits into from
Apr 8, 2024

Conversation

taartspi
Copy link
Collaborator

@taartspi taartspi commented Apr 1, 2024

…as chip 0. After initial export of a channel, sleep quarter second so SSD card compllets directory and individual device file creation. Bug fix when user calls getActualFreq prior to setting PWM channel ON.

…as chip 0. After initial export of a channel, sleep quarter second so SSD card compllets directory and individual device file creation. Bug fix when user calls getActualFreq prior to setting PWM channel ON.
…d 99% at 30. Set to 70 ms. Also, fixed bug if application with no pwm config shutdown value call pwm.shutdown(), then pi4j shutdown would invoke unexport a second time
@@ -44,7 +44,8 @@ public class LinuxPwm {
public static String DEFAULT_SYSTEM_PATH = "/sys/class/pwm";

/** Constant <code>DEFAULT_PWM_CHIP=0</code> */
public static int DEFAULT_PWM_CHIP = 0;
/** In Pi5 the chip is number 2 */
public static int DEFAULT_PWM_CHIP = 2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

test comment

@@ -85,13 +85,18 @@ public Pwm initialize(Context context) throws InitializeException {
if(!pwm.isExported()) {
logger.trace("exporting PWM [" + this.config.address() + "]; " + pwm.getPwmPath());
pwm.export();
// Delay to allow the SSD to persist the new directory and device partitions
Thread.sleep(250);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed? Maybe the export method need to do this check? What fails without this sleep?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the push last night the delay is 70 ms, a bit less pain. The app does pwm.on()x,y); If an export was not previously done, the code does an export and immediately call to set the period and calls to set the cycle. Setting the period fails with an access violation. In the export method we don't what will occur next and the delay fixes the exception. My assumption is the SSD has not completed the creation of the pwmX directory and contained files. I used this code very little on my Pi4 long ago and it worked. When we have system aware code in use I can verify that the Pi4 does not have a problem with no delay. Why 70 ms. 25 failed always, 30 worked nearly everytime. So although 35 might have worked with my fast SSD I doubled that to 70 assuming users with lesser quality SSD will need a greater delay.

Copy link
Member

Choose a reason for hiding this comment

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

The export will create a virtual directory in the kernel's memory space. This should not have anything to do with the attached storage. But it is certainly possible, that it takes the kernel a few milliseconds longer to configure the hardware for this, so i guess we'll keep these 70ms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok Stay 70. I show my need to learn more on this. Sometime maybe i will do more kernel learning to understand what is syncronous and also how the storage is really used

@@ -44,7 +44,8 @@ public class LinuxPwm {
public static String DEFAULT_SYSTEM_PATH = "/sys/class/pwm";

/** Constant <code>DEFAULT_PWM_CHIP=0</code> */
public static int DEFAULT_PWM_CHIP = 0;
/** In Pi5 the chip is number 2 */
public static int DEFAULT_PWM_CHIP = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean, that it won't work on the Pi4 anymore? Shouldn't we be doing a check for the environment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So yes. When franks Context update expose the machine type i can set the chip number correctly

@eitch eitch merged commit ae2ee8e into Pi4J:develop Apr 8, 2024
1 check passed
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.

2 participants