Skip to content

Have platform-dependent code in only one place #19

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

Closed
piranna opened this issue Jan 1, 2014 · 18 comments
Closed

Have platform-dependent code in only one place #19

piranna opened this issue Jan 1, 2014 · 18 comments
Labels
ports Relates to multiple ports, or a new/proposed port rfc Request for Comment

Comments

@piranna
Copy link

piranna commented Jan 1, 2014

Have a "platform" folder with subdirs for each platform with their platform dependent code inside (i.e., no asembler code on the core) like PyMite does. This will make more clear where to look and how to port it to another platforms, and also would easier to have a unique central makefile (just define as a param what platform do you want to compile).

@piranna
Copy link
Author

piranna commented Jan 1, 2014

Also would be nice to have to code stored on subfolders for each task to identify it better: lexer, parser, runtime...

@pfalcon
Copy link
Contributor

pfalcon commented Jan 1, 2014

I love structures and hierarchies, but then I know that you can go "too much" with it. Lexer and parser is ~1 file each. Having bunch of subdirs means extra work navigating among them. I'm not saying stuff can't be improved, it just should be balanced with practicality.

@KeithJRome
Copy link

The most practical thing is to organize it well. I'm not aware of this "navigation cost". Folders are free, and development IDEs don't care.

And just because it is a single file today doesn't mean it will always be a single file. In fact, a single code file for a lexer or parser for any dynamic language sounds VERY overweight at first blush.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 1, 2014

development IDEs don't care.

Yeah, but real men don't use finicky "ides", they use cd + vi, right? ;-)

@piranna
Copy link
Author

piranna commented Jan 1, 2014

Well, for that one file cases we can keep going this way, only that in fact
they are two: .c & .h. Anyway, the biggest problem is for assembler and
platform dependent code: they must go to their own folder.

Send from my Samsung Galaxy Note II
El 01/01/2014 19:08, "Paul Sokolovsky" notifications@github.com escribió:

development IDEs don't care.

Yeah, but real men don't use finicky "ides", they use cd + vi, right? ;-)


Reply to this email directly or view it on GitHubhttps://github.com/dpgeorge/issues/19#issuecomment-31427032
.

@dpgeorge
Copy link
Member

dpgeorge commented Jan 2, 2014

Agree that assembler code should go in a platform-specific folder.

@tylerwilson
Copy link

Just my 2 cents: I am not a huge fan of making platform folders, especially for code that is designed to be portable in the first place. Too much code duplication happens in my experience. Better to try to use a set of #defines and macros first.

Even for the assembler, if we can do #defines there and use macros, that would be preferable. For example, the porting of the nlrx64.S to OS X was simply a removal of some unsupported keywords. If we made those into defines we ought to be able to use one file for multiple platforms and not duplicate the majority of the code in different places.

@piranna
Copy link
Author

piranna commented Jan 2, 2014

Even for the assembler, if we can do #defines there and use macros, that
would be preferable. For example, the porting of the nlrx64.S to OS X was
simply a removal of some unsupported keywords. If we made those into
defines we ought to be able to use one file for multiple platforms and not
duplicate the majority of the code in different places.

Obviously, this multi-platform, half-portable code could go on the core
folder, and the platform specific files would only need to set #defines
that enabled them... Anyway, assembler code is not portable by definition,
so although it could be used by several platforms, it should be removed
from core folder (although not necesarily from the core code) or
substituted by an equivalent C code, and in my opinion anything that would
require a platform-dependent #define.

"Si quieres viajar alrededor del mundo y ser invitado a hablar en un monton
de sitios diferentes, simplemente escribe un sistema operativo Unix."
– Linus Tordvals, creador del sistema operativo Linux

@iabdalkader
Copy link
Contributor

This is very interesting to me as I want to start porting the code to the STM32F407 but not sure how the code should be structured, but here's an idea, it looks like each driver has two parts, the micropython binding, which is platform independent, I think, and a platform-dependent low-level I/O code, it seems like the first step would be to split each driver into a micropython binding and a target specific implementation of the low level stuff ?

take for example the i2c driver, there will be an stm/i2c_pyobj.c file and an stm/STM32F405/i2c_impl.c, this folder will also contain the STM32F405 linker script, so the hierarchy looks something like this:

stm/
|--i2c.h
|--i2c_pyobj.c  <--micropython binding
|--STM32F405/   
   |---stm32f405.ld   <-- linker script
   |---i2c_impl.c       <-- i2c lowlevel stuff
|--STM32F407/
   |---stm32f407.ld
   |---i2c_impl.c

and so on, now the Makefile would be changed to accept a target on the command line, and based on that it links that target's implementation files, so external calls in the *_pyobj.c would be resolved to the target specific implementation...

@pfalcon
Copy link
Contributor

pfalcon commented Jan 3, 2014

Few comments here:

I want to start porting the code to the STM32F407

Given your example, are you seriously thinking that F407 would have different I2C implementation than F405? Can't be that way unless proven otherwise (then STM worth an entry in some "f$#^^ up design" hall-of).

the micropython binding, which is platform independent
stm/i2c_pyobj.

If it's "platform independent", why it's under "stm/" ?

All in all, I'd say there's no need to have http://en.wikipedia.org/wiki/Analysis_paralysis , best practice is to start implementing new stuff in such a way as to require minimal changes to existing stuff, and then take further action based on actual experience with that. For example, I'd expect F407 port to require ~zero duplication of code, just adding drivers for new hardware blocks and parametrizing existing code (i.e. #ifdef's) - for example, for case like F407 has 3 I2C blocks, while F405 - 2 (even that's unlikely per my crystal ball).

@iabdalkader
Copy link
Contributor

I'm not sure, it probably doesn't have a different implementation, but it might have a different pin mapping ? that and the fact that it has a different memory layout, which means there's a different linker script that needs to go somewhere...
and also consider other families that will most likely have different implementations, e.g. STM32F3xx, now, are you still for using #ifdef for every micro in every family of micros to be supported ? can you "under analyze" something :D

The micropython binding looks platform independent, someone more familiar with the code might be able to confirm this, if so, then it needs to be separated from the implementation, ok maybe stm/ is a bad example.

@hagenkaye
Copy link
Contributor

#26 (comment)

This is how eLua implemented different platforms.

@Neon22
Copy link
Contributor

Neon22 commented Jan 4, 2014

Looking at the data sheets the 407 is a minum 100 pin device vs the 405's 64 pins.
So all pins will be different and must be re-mapped.
But internally alomst everything else is the same, just more ports and extra camera functionality.
So IMHO it could inherit from 405 but with changes to pin mapping, etc
This 'improvement' could be done as an optimisation when we get 405 working ? In the meantime (and possibly forever) it could be its own folder.

The elua layout is interesting. See https://github.com/elua/elua/blob/master/src/platform/stm32f4/stm32f4xx.h and compare with the adjacent folder for the stm32f2

@dpgeorge
Copy link
Member

As for the F407, @Neon22 is correct: it's basically the same inside, just has more pins and hence more GPIO and peripherals.

The STM library is the same for all F4's, and that's the one I'm using in the stm directory. So there should be almost no change to the code to get it running on an F407.

If anything, we just have some #define etc directives to enable additional I2Cs (or whatever is available) and remap the GPIO, LEDs, switches, etc.

I don't want to get bogged down trying to work out a perfect file structure etc for ports. I need to work on the pyboard port and make sure the pyboard works very well and has all the features that it should.

@iabdalkader
Copy link
Contributor

Speaking of the STM libraries and portability, I think an old version is used, the system_stm32f4xx.c has some code missing, for example in the latest version (V1.3) there are a few conditionals for setting the clock dividers based on the micro:

#if defined (STM32F40_41xxx) || defined (STM32F427_437xx) || defined (STM32F429_439xx)      
    /* PCLK2 = HCLK / 2*/
    RCC->CFGR |= RCC_CFGR_PPRE2_DIV2;

    /* PCLK1 = HCLK / 4*/
    RCC->CFGR |= RCC_CFGR_PPRE1_DIV4;
#endif /* STM32F40_41xxx || STM32F427_437x || STM32F429_439xx */

#if defined (STM32F401xx)
    /* PCLK2 = HCLK / 2*/
    RCC->CFGR |= RCC_CFGR_PPRE2_DIV1;

    /* PCLK1 = HCLK / 4*/
    RCC->CFGR |= RCC_CFGR_PPRE1_DIV2;
#endif /* STM32F401xx */

The one in stm/ doesn't have that..it might be wise to update at some point, same applies to the USB libraries.

@dhylands dhylands mentioned this issue Jan 17, 2014
@dpgeorge
Copy link
Member

The one in stm/ doesn't have that..it might be wise to update at some point, same applies to the USB libraries.

Thanks @iabdalkader for the hint: I've now upgraded the peripheral libraries to V1.3.0.

I'm using V2.1.0 for the USB drivers. Is there a newer one?

@iabdalkader
Copy link
Contributor

I'm using V2.1.0 for the USB drivers. Is there a newer one?

No, that's the latest one :)

@dpgeorge
Copy link
Member

dpgeorge commented Oct 3, 2014

Closing because this issue is out of date. The structure of the uPy repo is now more mature and has proven itself (so far), so any issues should be directed at its current state.

@dpgeorge dpgeorge closed this as completed Oct 3, 2014
drrk pushed a commit to drrk/micropython that referenced this issue Jan 22, 2017
retryfail pushed a commit to retryfail/micropython that referenced this issue Aug 20, 2020
tools/ftpserver.c: Refactoring of ftpserver.c
tannewt referenced this issue in tannewt/circuitpython Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ports Relates to multiple ports, or a new/proposed port rfc Request for Comment
Projects
None yet
Development

No branches or pull requests

8 participants