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

Update SPI signal terms to COPI / CIPO #215

Merged
merged 2 commits into from
Oct 24, 2020

Conversation

attie
Copy link
Member

@attie attie commented Oct 10, 2020

As discussed in #glasgow, this patch should update the SPI signal terms to COPI / CIPO / CS.

The old terms will no longer be obeyed, and flags such as --pin-copi must be used from now on.

I believe I've resolved all references to the old terms, but if there are other places (perhaps implicit) that I've missed please let me know.

Happy to squash into one commit if that would be preferred.

@attie
Copy link
Member Author

attie commented Oct 10, 2020

... I've just realised that the class is SPIMasterApplet for example - should that be renamed to SPIControllerApplet?... probably yes?

@whitequark
Copy link
Member

... I've just realised that the class is SPIMasterApplet for example - should that be renamed to SPIControllerApplet?... probably yes?

Yes. That's one of the implicit references.

@attie
Copy link
Member Author

attie commented Oct 10, 2020

I've been through a bit more thoroughly now - tests pass, and hopefully I've addressed everything.

@thasti
Copy link

thasti commented Oct 12, 2020

Took a look at this out of curiosity: This one line still contains a reference to "master" and MISO.

https://github.com/attie/glasgow/blob/spi-signals/software/glasgow/applet/interface/spi_controller/__init__.py#L329

@attie
Copy link
Member Author

attie commented Oct 12, 2020

@thasti - good catch, thanks for that.

@whitequark - can you provide more context about the clock_name parameter here? I suspect the name should be left as "master"...?

@whitequark
Copy link
Member

I suspect the name should be left as "master"...?

This is actually just a bad name because it's unclear what it even refers to, as you've just discovered. It's the clock that gets output on the SCK pin. Maybe something like "interface" would be more clear?

@attie
Copy link
Member Author

attie commented Oct 12, 2020

I see... it doesn't appear to be referenced anywhere (I was partially expecting it to appear as a clock domain - i.e: m.d.master), but it doesn't appear to.

If it's the generated / output clock, would simply "sck" suffice?

@attie
Copy link
Member Author

attie commented Oct 12, 2020

I have a feeling that this test failure is unrelated to the changes... (known issue? can I re-run without pushing a change?)

**********************
* Accellerated build *
**********************
warning: no previously-included files matching '*.pyc' found anywhere in distribution
warning: no previously-included files matching '*.cache' found anywhere in distribution
warning: no files found matching 'yarl/_quoting_c.c'
warning: no previously-included files found matching 'yarl/*.html'
warning: no previously-included files found matching 'yarl/*.so'
warning: no previously-included files found matching 'yarl/*.pyd'
no previously-included directories found matching 'docs/_build'
gcc: error: yarl/_quoting_c.c: No such file or directory
gcc: fatal error: no input files
compilation terminated.
error: Setup script exited with error: command 'gcc' failed with exit status 1
The command "(cd software && python setup.py install)" failed and exited with 1 during .
Your build has been stopped.

@whitequark
Copy link
Member

If it's the generated / output clock, would simply "sck" suffice?

It only appears in logs. "sck" works.

@whitequark
Copy link
Member

whitequark commented Oct 12, 2020

I have a feeling that this test failure is unrelated to the changes... (known issue? can I re-run without pushing a change?)

I've never seen this failure before. Frankly I have no idea what any of this means. Do we use "yarl"? What is it and why do we use it?

@whitequark
Copy link
Member

Do we use "yarl"? What is it and why do we use it?

Okay, seems like that's an URL parser with a C extension (cursed) and it broke because some of our dependencies have been updated. This seems like an upstream bug in yarl.

@attie
Copy link
Member Author

attie commented Oct 12, 2020

Okay, seems like that's an URL parser with a C extension (cursed) and it broke because some of our dependencies have been updated. This seems like an upstream bug in yarl.

Gah, okay... I should have dug first.

@whitequark
Copy link
Member

@attie Just to check, these commits don't work in isolation, right? I.e. the codebase is broken in between the first and the last one.

@attie
Copy link
Member Author

attie commented Oct 24, 2020

Yes, correct. Happy to squash them if you'd prefer.

@whitequark
Copy link
Member

Yup, please squash them so that the history is bisectable, then we could merge it. (The last one is unrelated though.)

@attie
Copy link
Member Author

attie commented Oct 24, 2020

Ok. Would you like a separate PR for the last commit, or can it be part of this PR and not squashed?

Any preference on the subject line prefix?

Just spi:?

@whitequark
Copy link
Member

Any preference on the subject line prefix?

For sweeping changes like that I simply use a descriptive commit line without any formal prefix. After all, rules are made to be broken.

@whitequark whitequark merged commit b769470 into GlasgowEmbedded:master Oct 24, 2020
@whitequark
Copy link
Member

Thanks for doing this!

@attie
Copy link
Member Author

attie commented Oct 24, 2020

No problem! :-)

@attie attie deleted the spi-signals branch October 24, 2020 18:42
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

5 participants