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.

Sign up for GitHub

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

Jump to bottom

SPI still available? #99

Closed
enelson1001 opened this issue Oct 2, 2019 · 31 comments
Closed

SPI still available? #99

enelson1001 opened this issue Oct 2, 2019 · 31 comments
Labels
Discussion

Comments

@enelson1001
Copy link
Contributor

I noticed that lib/smooth/include/smooth/io does not contain spi. Have you removed support for SPI?
@PerMalmberg
Copy link
Owner

PerMalmberg commented Oct 2, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 2, 2019 via email

@PerMalmberg
Copy link
Owner

I don't remember the API exactly, but weren't there two sides of it? Merging them into one sounds like creating a God-object...

@squonk11
Copy link
Contributor

squonk11 commented Oct 3, 2019

Just a hint from my side: the slave mode of the ESP32 has a design flaw. Due to this there is the following restriction: master should write lengths which are a multiple of 4 bytes. Data longer than that will be discarded; see here: known issues.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 3, 2019 via email

@PerMalmberg
Copy link
Owner

Remembered that I have have e-paper displays that work over SPI. Not sure how much time I have left over in the near future but I'm up for some discussions around this.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 4, 2019 via email

@PerMalmberg
Copy link
Owner

  1. I have no idea, but I don't remember it doing that when I wrote the code.

  2. Are you saying my old code you dug up uses .user to to point to the driver itself?

I suppose what you're really after is the ability to set the DC line in this function?

void lcd_spi_pre_transfer_callback(spi_transaction_t *t)
{
    int dc=(int)t->user;
    gpio_set_level(PIN_NUM_DC, dc);
}

I've litterally just given it 3 minutes of thought, but if you can use t as a key, you could do a lookup for the DC value via the driver instance. Just have to keep in mind that the function is called in ICQ-context.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 5, 2019 via email

@PerMalmberg
Copy link
Owner

  1. You should leave that line active it enables sane warning options. A better options is to do like on line 24 and 28 for that warning.

  2. You have the entire array of transactions currently being sent available in the object pointed to by this, right? If so, wrap each transaction, something like this:

struct MyTransaction
{
    spi_transaction_t t;
    bool dc_state;
}

In the driver:

MyTransaction data_to_send[N];

For each transaction, make the userpointer point to itself:

data_to_send[i].t.user = &data_to_send[i];

Then, we can do this:

void lcd_spi_pre_transfer_callback(spi_transaction_t *t)
{
    auto transaction = reinterpret_cast<MyTransaction*>(t->user);
    gpio_set_level(PIN_NUM_DC, transaction->dc_state);
}

Big disclaimer, this is just brain dump :)

@PerMalmberg
Copy link
Owner

Actually, I presume you need an array of spi_transaction_t so you can't wrap it. So instead place dc_state as a separate array:

In the driver:

std::array<bool, N> dc_state{};
std::size_t current_transaction{};

For each transaction, make the userpointer point to the driver

data_to_send[i].user = this;

Then, we can do this:

void lcd_spi_pre_transfer_callback(spi_transaction_t *t)
{
    auto driver = reinterpret_cast<Driver*>(t->user);
    gpio_set_level(PIN_NUM_DC, driver->dc_state[driver->current_transaction]);
    driver->current_transaction++;
}

Big disclaimer, this is just brain dump :)

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 5, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 5, 2019 via email

@PerMalmberg
Copy link
Owner

(use regular markdown syntax to format code etc)

Yes, I think my second suggestion should work for you.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 5, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 7, 2019 via email

@PerMalmberg
Copy link
Owner

Glad to hear you're making progress. 👍

  1. I honestly don't remember; I never got very far with the original SPI implementation since I went with all I2C in on the hardware side for my project. Does it make sense to place the methods there? Or, in other words, does it feel natural to do my_device.queue_transaction? Is the code up on GH so I can look at it?

  2. What's it used for? iirc, portMAX_DELAY means infinite timeout, i.e. blocking operation. I've tried to use std::chrono::milliseconds for timeouts in Smooth when possible.

  3. If its needed, is there another way? what does it do?

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 7, 2019 via email

@PerMalmberg
Copy link
Owner

Sounds like things are falling into place.

Look in I2CMasterDevice.h for the to_tick function for converting from ms to ticks.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 7, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 8, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 8, 2019 via email

@PerMalmberg
Copy link
Owner

core::Task has two uses: As a standalone task, or to run as the main task (which it was it does when used via the App class.

If you just want to create a new Task, then look at hello_world (or nearly any other example) which creates a secondary worker task.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 10, 2019 via email

@PerMalmberg
Copy link
Owner

Can SPI list IDs of devices on the bus, like I2C can? If so, that'd be a nice and simple demo/test project.

@PerMalmberg
Copy link
Owner

FYI: There are two major changes in master now:

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 17, 2019 via email

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 28, 2019 via email

@PerMalmberg
Copy link
Owner

Can you please fork the original repo and create a PR for things going into Smooth? That'll clearly show what changes you've made.

Is it possible to use the same relation between spi::Master and an spi-device like there is between i2c::Master and i2c::MasterDevice, where the Master acts like a factory for the device?

Re. BME280. As it seems to be a near copy of the ic2BME280, I think they should either share a base class, or use a shared helper class that performs the calculations etc. Also, there were recently some bug fixes to the i2c::BME280 implementation, make sure those are included of they aren't already.

@enelson1001
Copy link
Contributor Author

enelson1001 commented Oct 30, 2019 via email

@PerMalmberg
Copy link
Owner

You could also place all the common things in a BME280Common or some such, that'd avoid the multiple inheritance. Where to place it...well, I think it's fine if it lives in either i2c or spi, no need to create a directory just for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@PerMalmberg @squonk11 @enelson1001