improve hal_apply_direction/1 for pins not activated by simply exporting #116
Conversation
WHY: not rely on the value of direction after export Because there are pins that have a value for direction after export, but do not operate according to that value. These pins will not be activated unless they are overwritten again, even if they have the same direction value after export. Therefore, modified it to write regardless of the value of direction.
| /* Linux only reports "in" and "out". (current_dir is NOT null terminated here) */ | ||
| int current_is_output = (current_dir[0] == 'o'); | ||
| int retries = 1000; | ||
|
|
||
| if (pin->config.is_output == 0) { | ||
| // Input | ||
| if (!current_is_output) | ||
| return 0; | ||
| else | ||
| return sysfs_write_file(direction_path, "in", 0); | ||
| return sysfs_write_file(direction_path, "in", retries); |
There was a problem hiding this comment.
For example, after export the pin's direction is set in default.
In that case current_is_output has falthy value, then do not reach to sysfs_write_file(direction_path, "in", 0).
So the pin that is not activated by just exporting will never be activated again unless its direction is changed.
There was a problem hiding this comment.
Unfortunately, writing "out" has a side effect of changing the GPIO value. That's why the original code works the way that it does.
For your issue, I think that it should be sufficient to only make the change that unconditionally writes "in" for inputs:
if (pin->config.is_output == 0) {
// Input
return sysfs_write_file(direction_path, "in", 0);
} else {Would this fix the issue that you're seeing?
As a side comment, I think that the real problem is that Linux is misreporting the pin direction and that's what needs to be fixed. However, I don't think there's any harm in always writing "in" like there is when writing "out".
There was a problem hiding this comment.
Thanks for your quick reply.
Unfortunately, writing "out" has a side effect of changing the GPIO value.
I tested and understood what you mean. Thank you for pointing this out.
Would this fix the issue that you're seeing?
Yes, but...
I think that the real problem is that Linux is misreporting the pin direction and that's what needs to be fixed.
You mean just after exporting, Linux should report the correct default direction? (in my case it should be "out" but "in" is reported. If so, I would not like to change circuits_gpio for the wrong report.
I don't know if I can do it, but I would like to look into the Linux side of things, why the wrong report is happen.
There was a problem hiding this comment.
You mean just after exporting, Linux should report the correct default direction? (in my case it should be "out" but "in" is reported. If so, I would not like to change circuits_gpio for the wrong report.
Right, Linux should be right. I really don't think that it's worth trying to fix Linux unless cdev has the same issue. Circuits.GPIO really needs to be updated to use the cdev interface rather than sysclass. It would not be good to spend time on a sysclass issue.
I don't know if I can do it, but I would like to look into the Linux side of things, why the wrong report is happen.
I honestly think that modifying your PR to only write "in" is ok. I don't think there are any downsides and it works around your issue. Of course, if it were me, I'd also spend some time looking at the Linux kernel since if there's a bug here, maybe there's another bug that cannot be worked around so easily.
It's up to you. I'll accept a PR that fixes "in" if you don't have time to find out why the Linux kernel reports the wrong value.
There was a problem hiding this comment.
Right, Linux should be right. I really don't think that it's worth trying to fix Linux unless cdev has the same issue.
I tried circuits_cdev for my issue, the results is following, so this should be Linux side issue.
iex(4)> {:ok, line_handle} = Circuits.Cdev.request_line("gpiochip0", 26, :input)
{:ok,
%Circuits.Cdev.LineHandle{
chip: %{
label: "GPIOA",
name: "gpiochip0",
number_of_lines: 16,
reference: #Reference<0.3144325637.108920833.190232>
},
handle: #Reference<0.3144325637.108920833.190233>
}}
iex(5)> Circuits.Cdev.read_value(line_handle)
{:error, :read_values_failed}
It's up to you. I'll accept a PR that fixes "in" if you don't have time to find out why the Linux kernel reports the wrong value.
Fortunately, this issue is not urgent and I will take my time to investigate. So I will close this PR after you read this reply.
Thank you always.
There was a problem hiding this comment.
Thanks. Good luck with Linux.
There was a problem hiding this comment.
am sorry, I made a mistake to get the result of cdev, i missed the offset of gpiochip.
I tried read again with correct setting with cdev, it works. here is the result,
(gpiochip0, offset 26 is wrong setting, the gpiochip of the board has 16 line for each chip, so I have to use gpiochip1, offset 10 setting)
iex(2)> {:ok, line_handle} = Circuits.Cdev.request_line("gpiochip1", 10, :input)
{:ok,
%Circuits.Cdev.LineHandle{
chip: %{
label: "GPIOB",
name: "gpiochip1",
number_of_lines: 16,
reference: #Reference<0.2538447730.2718564354.227492>
},
handle: #Reference<0.2538447730.2718564354.227493>
}}
iex(3)> Circuits.Cdev.read_value(line_handle) # not push button
{:ok, 0}
iex(4)> Circuits.Cdev.read_value(line_handle) # push button
{:ok, 1}
So Could I reopen this PR for mergin PR that fixes "in", you mentioned here, #116 (comment) ?
There was a problem hiding this comment.
I don't have the permission to reopen. Could you do that? or If can't, please let me know, I make PR again.
|
It appears that I can’t reopen it either! Sorry about this. Can you open a new PR? |
WHY: not rely on the value of direction after export
Because there are pins that have a value for direction after export,
but do not operate according to that value.
These pins will not be activated unless they are overwritten again,
even if they have the same direction value after export.
Therefore, modified it to write regardless of the value of direction.
Closes b5g-ex/nerves_system_stm32mp157c_odyssey#3