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

I2ctarget doc function request: a) mods to doc text & function signature. b) small mod of the code #6985

Merged
merged 8 commits into from Oct 3, 2022

Conversation

PaulskPt
Copy link

@PaulskPt PaulskPt commented Oct 3, 2022

Intro

First a remark to the choice of the name of my branch 'i2ctarget_doc_text_mod'. This title was chosen because, initially, the (first) modification suggested in this PR was limited to the doc text of the function 'request' of the class 'I2CTarget' and the function signature inside that text.
As I will explain below, in this moment, the title of this PR is broader. This PR also contains a suggestion for a small change into the code of the function 'request'.

Subject 1: Initial error

While busy with a test script using the I2CTarget class, a run of my script, when calling the function 'request', resulted in the error 'Extra positional arguments given'

On October 1, at 1:58 pm (utc +1) I posted a question on Discord > Adafruit > circuitpython-dev:

2022-10-01_13h48_I2CTarget request_error

@jepler answered to my question. He explained me that his was caused because of the function signature:

I2CTarget request_function_signature_original

@jepler showed me that the following change would solve the error:

I2CTarget request_doc_txt_change_suggestion

At the end of our conversation @jepler showed me a REPL test output as well as his suggestion to me: 'An issue, or better yet a PR to fix it, would be appreciated from you once you've verified it as well. '

2022-10-01_14h16_Discord_Adafruit_CPY-dev_2nd_reply_fm_jepler

As result of the conversation with @jepler I suggest the following change in the function signature of function 'request':

I2CTarget request_doc_txt_change_suggestion

Subject 2: range parameter timeout

Today, October 3, 2022, I was studying the code of file I2CTarget.c
My attention went in particular to: Class I2CTarget, function: 'request'

While the conversation with @jepler on Discord is the reason for this PR and the suggested change is concerning the doc text preceding the function 'request' and a change of the function signature, today I discovered that the definition of function 'request' defaults parameter 'timeout to -1'. However, the code inside the function checks timeout in the 'if block' for value 0 and - in the 'else if' block - for values > 0. (see below). While the function defaults parameter 'timeout' to -1, the block: ```

if (timeout_ms == 0) {
        forever = true;
    } else if (timeout_ms > 0) {
        timeout_end = common_hal_time_monotonic_ms() + timeout_ms;
    }

does not check for parameter timeout values < 0.
That is why I suggest the following change: ```

if (timeout_ms <= 0) {
        forever = true;
    } else if (timeout_ms > 0) {
        timeout_end = common_hal_time_monotonic_ms() + timeout_ms;
    }

truth_table

The result of my modification is that parameter 'timeout' now is checked for a full range of possible float values:

(float max negative) <= 0 > (float max positive)

while the current code checks a limited range of values for timeout:

0 > (float max positive)

Final note: concerning the first commit to this branch

To be able to create this PR, online I created a fresh fork of Circuitpython. Next, using the MS Windows 11 Github desktop app, I created a local clone of the fork. Then I created this branch. After the Github desktop app was ready with cloning and arranging things, the app immediately confronted me with a 'change' it assumed I had done in file /ports/broadcom/firmware.

uncrustify_initial

However I did not do such a thing. Since I am not very familiar with the ins and outs of Github commands regarding cloning, creating branches and so on, I did not know what to do with it. Today I discovered that the possible reason for Github desktop app to tell me I made a change in /ports/broadcom/firmware, was that I saw this file being mentioned at the end of the '.gitignore' under 'uncrustify'.
I removed the filename '/ports/broadcom/firmware' entry there, resulting in:

uncrustify

Now the contents of the .gitignore file in this branch (at least the last few lines of it) is equal to the contents of the .gitignore file of the 'main' branch of the 'circuitpython repo.

This ends my description of the reasons for creating this PR and the examples/clarifications to the matter.
I thank @jepler for his replies, assistance and to 'push' me (kindly) to create this PR.

Lisbon, October, 3, 4:16 pm local (utc +1),
Paulus Schulinck, @PaulskPt

PaulskPt and others added 8 commits October 1, 2022 21:34
I made a fresh fork of circuitpython. Using Github Desktop app, created a local clone of this fork. Then Desktop app confronted me we a change in ports\broadcom\firmware -Subproject commit .nr.... and +Subproject commit same nr-dirty.
Modification of the doc text of function request().

1) The timout parameter is a keyword-only argument; so Added '*,' in the function signature;
2) for parameter timeout an integer is expected, not a float.
Correction in function request() doc function signature.
(after speaking with @jepler on Discord).

(@jepler: 'Circuitpython always has floats enabled')
Correction of the timeout value range needed to set the timeout to 'forever'.
The line 162 checks timeout for a value of 0 while the function definition defaults timeout to -1. In the current version of the code timeout is only checked for a value of 0 or in the 'else if' part for a value of > 0. So, values of <0 will not be taken in to account.  That is the reason of my modification.
Removed the -# Uncrustify formatting for file
ports/broadcom/firmware

I don't know how it came into there. I didn't put it.
It is not my intention to change anything else than this branch to make 2 changes in  shared_bindings/I2CTarget module
I made an error. I deleted too much at the end of this file. Correction made. It is now (the Uncrustify formatting) as in branch 'main'
Pre-commit adjusted some line/file endings
This item is a UNIX symbolic link, and (except in the unlikely case
where the symlink is to a pathname that ends with a newline character!)
doesn't contain a newline.

It appears some well-intentioned tool failed to correctly handle this
file, and added a trailing newline as though it was a text file.
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks! I fixed tests/pyboard.py which was incorrectly modified as part of this PR.

@jepler jepler merged commit abd0228 into adafruit:main Oct 3, 2022
@PaulskPt PaulskPt deleted the i2ctarget_doc_text_mod branch October 3, 2022 21:40
@PaulskPt
Copy link
Author

PaulskPt commented Oct 3, 2022

Again thank you Jeff.

@s-ol
Copy link

s-ol commented Nov 20, 2022

Hey, I think the check timeout == 0 and timeout > 0 is intentional: -1 (do not block) is not the same as 0 (block indefinitely).

When timeout is -1, forever should be false. This change should be reverted to resolve #7241 and restore functionality as documented:

timeout (float) - Timeout in seconds. Zero means wait forever, a negative value means check once

s-ol added a commit to s-ol/circuitpython that referenced this pull request Nov 20, 2022
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.

None yet

3 participants