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

shared-module/fourwire/FourWire.c: make the chip_select pin optional #9106

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

Fabien-Chouteau
Copy link

If there's only one device on the bus, the chip_select pin of the peripheral can be fixed in hardware, therefore lowering the number of pins required on the microcontroller side.

This patch allows this by making the chip_select pin optional.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

This doesn't look finished. There are a number of places in shared-module/fourwire/FourWire.c where self.chip_select is referenced and assumed not to be None, e.g. line 83, and a bunch of others.

If there's only one device on the bus, the chip_select pin of the
peripheral can be fixed in hardware, therefore lowering the number of
pins required on the microcontroller side.

This patch allows this by making the chip_select pin optional.
@Fabien-Chouteau
Copy link
Author

I'm sorry @dhalbert, I am not familiar with the codebase and I should've spend more time on this.

I added checks for the chip_select pin.

@Fabien-Chouteau Fabien-Chouteau requested a review from dhalbert April 1, 2024 09:37
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for the update. One more place to test if chip_select pin is set or not:

void common_hal_fourwire_fourwire_deinit(fourwire_fourwire_obj_t *self) {
    if (self->bus == &self->inline_bus) {
        common_hal_busio_spi_deinit(self->bus);
    }

    common_hal_reset_pin(self->command.pin);
    common_hal_reset_pin(self->chip_select.pin);     //// *************
    common_hal_reset_pin(self->reset.pin);
}

@Fabien-Chouteau
Copy link
Author

Hi @dhalbert

Thanks for the update. One more place to test if chip_select pin is set or not:

Since the command pin can also be None and is not checked here, my understanding is that the common_hal_reset_pin will check if the pin is None:

void common_hal_reset_pin(const mcu_pin_obj_t *pin) {
    if (pin == NULL) {
        return;
    }
    reset_pin_number(pin->port, pin->number);
}

But maybe there is an oversight on the command pin as well.

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 5, 2024

Since the command pin can also be None and is not checked here, my understanding is that the common_hal_reset_pin will check if the pin is None:

This depends on self->chip_select being initialized to zero, so self->chip_select.pin will be NULL (zero) at first. I was worried about that at first, but I cannot think of a situation where the pin might be set to a non-NULL value and then later the chip_select type was set to NoneType, so I think it is ok.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@dhalbert dhalbert merged commit 8027efe into adafruit:main Apr 5, 2024
431 checks passed
@Fabien-Chouteau Fabien-Chouteau deleted the fourwire_optional_cs branch April 7, 2024 13:28
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