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

[board] Add basic support for STM32_F32VE board and [driver] ILI9341 via #464 #437

Merged
merged 4 commits into from
Sep 19, 2020

Conversation

asmfreak
Copy link
Contributor

@asmfreak asmfreak commented Jul 19, 2020

This PR add support for this board.

  • board.hpp actually works (ported from disco_f407vg board)
  • Examples ported
    • Blink ex
    • Uart works
    • LCD Display works (requires new functionality for ILI9341 in ParallelDisplay module).
    • Touch screen works
    • On-board memory unit works

@asmfreak asmfreak marked this pull request as draft July 19, 2020 19:26
@asmfreak asmfreak marked this pull request as ready for review July 19, 2020 19:26
@asmfreak asmfreak marked this pull request as draft July 19, 2020 19:27
// E��((X0��X2) (YD1��YD2)��(YD0��YD2) (X1��X2))��K */
En = ((sample[0].x - sample[2].x) * (display[1].y - display[2].y)) -
((display[0].y - display[2].y) * (sample[1].x - sample[2].x));
// F��(Y0(X2YD1��X1YD2)+Y1(X0YD2��X2YD0)+Y2(X1YD0��X0YD1))��K */
Copy link
Member

Choose a reason for hiding this comment

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

Encoding issues…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very strange. I've copied it from another example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Welp. Encoding is broken in examples/stm32f4_discovery/open407v-d/gui/touchscreen_calibrator.hpp.

Copy link
Member

Choose a reason for hiding this comment

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

I think this file should be part of the :ui:display module, so it can be reused by everyone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. We just need to fix the encoding somehow.

@salkinium
Copy link
Member

Nice! Are you sure you want to port all this functionality at once? I would recommend breaking it down into multiple PRs. Things like SD-Card support probably need a little more work since our FatFS implementation needs to be replaced, etc.

@asmfreak
Copy link
Contributor Author

Since I want to experiment with all possible things, maybe I should convert this PR into an issue, and work it from there?
I was hoping to get support for the screen (ILI9341), but it is still lacking, which, I guess, will be a separate PR.

@salkinium
Copy link
Member

maybe I should convert this PR into an issue, and work it from there?

It's fine, the difference between issue and PR is superficial. I just thought it might be easier to do it bit by bit, but it doesn't really matter that much.

@mikewolfram
Copy link
Contributor

Since I want to experiment with all possible things, maybe I should convert this PR into an issue, and work it from there?

I was hoping to get support for the screen (ILI9341), but it is still lacking, which, I guess, will be a separate PR.

I have IlI9341 implemented and working. Could submit it when returned from vacation.

@salkinium
Copy link
Member

Any updates on this?

@asmfreak
Copy link
Contributor Author

@mikewolfram I've added ILI9341 support here. Maybe your code could improve my rather crude version?

@asmfreak asmfreak force-pushed the feature/add_board_stm32_f4ve branch 2 times, most recently from 78607cd to 1a22120 Compare September 13, 2020 10:20
@mikewolfram
Copy link
Contributor

@mikewolfram I've added ILI9341 support here. Maybe your code could improve my rather crude version?

Sorry, had no time to finish it. I opened a draft PR so you can compare: #464. Wanted to improve it to support other interfaces than SPI too.

@asmfreak
Copy link
Contributor Author

I've merged #464 here and improved by adding parallel interface to it.
@mikewolfram Is it ok with you?

@asmfreak
Copy link
Contributor Author

asmfreak commented Sep 17, 2020

@salkinium Can you please point me to docs on how to properly add a new board to modm?
I guess one should edit some Makefiles to point build system towards new board/examples?

@salkinium
Copy link
Member

Can you please point me to docs on how to properly add a new board to modm?

Yes: https://modm.io/reference/module/modm-board/

I guess one should edit some Makefiles to point build system towards new board/examples?

No, a board is just an lbuild config + module, and gets auto-discovered inside the modm/board folder.
The way you did it with your board looks just fine, is it not working correctly?

@salkinium
Copy link
Member

Ah, you probably mean to the modm CI?

Just add the example folder name to the right list here: https://github.com/modm-io/modm/blob/develop/.circleci/config.yml#L131

@asmfreak
Copy link
Contributor Author

I'll test SD card, NRF24 and CAN support examples later.

@asmfreak asmfreak marked this pull request as ready for review September 17, 2020 16:25
@asmfreak asmfreak changed the title [board] Add basic support for STM32_F32VE board [board] Add basic support for STM32_F32VE board and [driver] ILI9341 via #464 Sep 17, 2020
@salkinium
Copy link
Member

(I misconfigured the CircleCI reporters, but it's fixed on the next push)

@salkinium
Copy link
Member

If you are adding all the .pbm files to the repository, it would make sense to make them part of the :ui:display module: https://github.com/modm-io/modm/tree/develop/src/modm/ui/display/image

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Where do the images come from and what licence do they have? If they're compatible I'd like to see them moved into the library for reuse.

I'm going to adapt the ILI9341 driver just a little bit to our coding style. It's easier this way than to review it.

// tft.drawImage(pixPos, pix);
}

return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why is

there

so much

space in this

function?

😜

// E��((X0��X2) (YD1��YD2)��(YD0��YD2) (X1��X2))��K */
En = ((sample[0].x - sample[2].x) * (display[1].y - display[2].y)) -
((display[0].y - display[2].y) * (sample[1].x - sample[2].x));
// F��(Y0(X2YD1��X1YD2)+Y1(X0YD2��X2YD0)+Y2(X1YD0��X0YD1))��K */
Copy link
Member

Choose a reason for hiding this comment

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

I think this file should be part of the :ui:display module, so it can be reused by everyone.

examples/stm32_f4ve/gui/images/.gitignore Outdated Show resolved Hide resolved
@asmfreak
Copy link
Contributor Author

examples/stm32_f4ve/gui code and images are coming from another example examples/stmf4_discovery/open407v-d/gui. So any questions concerning file encodings and image files licenses are to be asked about that example too. I've just copied that example to stm32_f4ve's folder and edited a bit to make it compile and run on this board.

@asmfreak
Copy link
Contributor Author

@salkinium What is my next step on this PR? Please advise.

@salkinium
Copy link
Member

If you are motivated, you could move all .pbm images inside the examples into the :display module.

But since I'm not super aware of the :display and :ui modules, since they were written almost exclusively by @dergraaf and @daniel-k, we can also just merge this as is, and sort this out later. I was mainly just concerned about repo size, but since these files are already in the repo, it doesn't matter much.

@salkinium
Copy link
Member

If you're not motivated, that's fine too (your contributions are more important than this optimization), then I'll squash the branch and fix commit structure to match our style.

@asmfreak
Copy link
Contributor Author

I think that :display and :ui modules refactoring should take place in another PR with proper discussion with @dergraaf and @daniel-k . I'd prefer for this PR to be merged as is.

Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Nice!
I ordered the Board & Display, looking forward to play with it (when it arrives in a few weeks).

@salkinium
Copy link
Member

Ok, I'll rebase/squash/restructure and adapt for @rleh's review.

@asmfreak
Copy link
Contributor Author

Nice!

Co-authored-by: delphi <cpp.create@gmail.com>
@salkinium
Copy link
Member

Just restructured logically, and added shared author-ship on IL9341 driver.

@salkinium salkinium merged commit a6f1186 into modm-io:develop Sep 19, 2020
@asmfreak asmfreak deleted the feature/add_board_stm32_f4ve branch September 19, 2020 17:19
if (i.releaseMaster())
Cs::set();
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really make sense to pass the SPI interface every time? Wouldn't it be better to do like:

	struct BatchHandle
 	{
 		BatchHandle()
 		{
 			SPI::acquireMaster();
 			Cs::reset();
 		}
 		~BatchHandle()
 		{
 			if (SPI::releaseMaster())
 				Cs::set();
 		}
	};

And maybe the name AcquireInterface is more intuitive to BatchHandle?

My few cents...

Copy link
Member

Choose a reason for hiding this comment

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

Yes probably. However the compiler will inline all of this anyways. Feel free to change this if you feel strongly about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants