persistentcode.c: save_raw_code() - add flush/finalizer callback

Lines 319 to 371 in e9d29c9

    
           STATIC void save_raw_code(mp_print_t *print, mp_raw_code_t *rc) { 
        
               if (rc->kind != MP_CODE_BYTECODE) { 
        
                   mp_raise_ValueError("can only save bytecode"); 
        
               } 
        
               // save bytecode 
        
               mp_print_uint(print, rc->data.u_byte.bc_len); 
        
               mp_print_bytes(print, rc->data.u_byte.bytecode, rc->data.u_byte.bc_len); 
        
               // extract prelude 
        
               const byte *ip = rc->data.u_byte.bytecode; 
        
               const byte *ip2; 
        
               bytecode_prelude_t prelude; 
        
               extract_prelude(&ip, &ip2, &prelude); 
        
               // save qstrs 
        
               save_qstr(print, ip2[0] | (ip2[1] << 8)); // simple_name 
        
               save_qstr(print, ip2[2] | (ip2[3] << 8)); // source_file 
        
               save_bytecode_qstrs(print, ip, rc->data.u_byte.bytecode + rc->data.u_byte.bc_len); 
        
               // save constant table 
        
               mp_print_uint(print, rc->data.u_byte.n_obj); 
        
               mp_print_uint(print, rc->data.u_byte.n_raw_code); 
        
               const mp_uint_t *const_table = rc->data.u_byte.const_table; 
        
               for (uint i = 0; i < prelude.n_pos_args + prelude.n_kwonly_args; ++i) { 
        
                   mp_obj_t o = (mp_obj_t)*const_table++; 
        
                   save_qstr(print, MP_OBJ_QSTR_VALUE(o)); 
        
               } 
        
               for (uint i = 0; i < rc->data.u_byte.n_obj; ++i) { 
        
                   save_obj(print, (mp_obj_t)*const_table++); 
        
               } 
        
               for (uint i = 0; i < rc->data.u_byte.n_raw_code; ++i) { 
        
                   save_raw_code(print, (mp_raw_code_t*)(uintptr_t)*const_table++); 
        
               } 
        
           } 
        
           void mp_raw_code_save(mp_raw_code_t *rc, mp_print_t *print) { 
        
               // header contains: 
        
               //  byte  'M' 
        
               //  byte  version 
        
               //  byte  feature flags 
        
               //  byte  number of bits in a small int 
        
               byte header[4] = {'M', MPY_VERSION, MPY_FEATURE_FLAGS_DYNAMIC, 
        
                   #if MICROPY_DYNAMIC_COMPILER 
        
                   mp_dynamic_compiler.small_int_bits, 
        
                   #else 
        
                   mp_small_int_bits(), 
        
                   #endif 
        
               }; 
        
               mp_print_bytes(print, header, sizeof(header)); 
        
               save_raw_code(print, rc); 
        
           }

mp_raw_code_save needs a finalizing call for the case of print being a function that saves to flash (or more generally, an implementation that needs a manual flush).

Flash can only be written one page at a time so unless all calls to print just happen to have a cumulative number of bytes equal to a multiple of the page size . . . print will be stuck waiting for the last (page_size_bytes - buffer_curr_num_bytes) bytes forever.

The simplest way to implement this flush is to call print->print_strn with NULL for data and 0 for length (unless someone can think of valid use for those set of arguments).

print->print_strn(print->data, (const char*)NULL, 0);

But the cleaner way would be to add a member to mp_print_t

micropython/py/mpprint.h

Lines 48 to 53 in e9d29c9

    
           typedef void (*mp_print_strn_t)(void *data, const char *str, size_t len); 
        
           typedef struct _mp_print_t { 
        
               void *data; 
        
               mp_print_strn_t print_strn; 
        
           } mp_print_t;

typedef void (*mp_print_strn_t)(void *data, const char *str, size_t len);
typedef void (*mp_print_flush_t)(void *data);

typedef struct _mp_print_t {
    void *data;
    mp_print_flush_t print_flush;
    mp_print_strn_t print_strn;
} mp_print_t;

And the very last statement in mp_save_raw_code is save_raw_code_flush(print)

void save_raw_code_flush(mp_print_t *print)
{
    if(print->print_flush!= NULL) print->print_flush(print->data);
}

If your implementation of mp_print_t *print doesn't need a finalizer, you can just set print->print_flush = NULL.

The function that calls save_raw_code() (actually that calls mp_raw_code_save()) should be responsible for doing any finalisation of the stream. For example, mp_raw_code_save_file() closes (and implicitly flushes) the file once done.

For example, mp_raw_code_save_file() closes (and implicitly flushes) the file once done.

I think you made my point with this example: I agree that it makes sense for mp_raw_code_save_file() to call close(fd). For that same reason, mp_raw_code_save() should also call the finalizer: either the finalizer is called by the caller of mp_raw_code_save*() or it's called inside said function.

I agree that it makes sense for mp_raw_code_save_file() to call close(fd). For that same reason, mp_raw_code_save() should also call the finalizer:

I don't see how this logically follows. mp_raw_code_save() is essentially a helper function and the caller of it should finalise the stream, because the caller knows best how to do finalisation.

mp_raw_code_save_file() calls the finalizer close() so why shouldn’t mp_raw_code_save() call a finalizer?

In the same way that mp_print_t provides the interface to “print” which needs to be set to an actual “print” function, mp_print_t should also contain an interface to finalize (as I mentioned in the first post to this issue)

typedef void (*mp_print_strn_t)(void *data, const char *str, size_t len);
typedef void (*mp_print_flush_t)(void *data);

typedef struct _mp_print_t {
    void *data;
    mp_print_flush_t print_flush;
    mp_print_strn_t print_strn;
} mp_print_t;

mp_raw_code_save_file() calls the finalizer close() so why shouldn’t mp_raw_code_save() call a finalizer?

Becasue mp_raw_code_save_file() can do it and there's no need to burden the core routine with a finaliser call.

In your specific case, it looks like it should work just fine to flush the output at the very end (ie not in mp_raw_code_save()).

Merge pull request micropython#3478 from ansonhe97/main

mp_raw_code_save() is a mid-level API function (eg you may want to call it twice in a row to save 2 separate raw codes). Higher-level API functions built on top of this function are responsible for flushing/closing the stream, eg:

void mp_raw_code_save_flash(...) {
    mp_raw_code_save(...);
    commit_to_flash();
}

Navigation Menu

Repository navigation

persistentcode.c: save_raw_code() - add flush/finalizer callback #3478

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	STATIC void save_raw_code(mp_print_t print, mp_raw_code_t rc) {
	if (rc->kind != MP_CODE_BYTECODE) {
	mp_raise_ValueError("can only save bytecode");
	}

	// save bytecode
	mp_print_uint(print, rc->data.u_byte.bc_len);
	mp_print_bytes(print, rc->data.u_byte.bytecode, rc->data.u_byte.bc_len);

	// extract prelude
	const byte *ip = rc->data.u_byte.bytecode;
	const byte *ip2;
	bytecode_prelude_t prelude;
	extract_prelude(&ip, &ip2, &prelude);

	// save qstrs
	save_qstr(print, ip2[0] \| (ip2[1] << 8)); // simple_name
	save_qstr(print, ip2[2] \| (ip2[3] << 8)); // source_file
	save_bytecode_qstrs(print, ip, rc->data.u_byte.bytecode + rc->data.u_byte.bc_len);

	// save constant table
	mp_print_uint(print, rc->data.u_byte.n_obj);
	mp_print_uint(print, rc->data.u_byte.n_raw_code);
	const mp_uint_t *const_table = rc->data.u_byte.const_table;
	for (uint i = 0; i < prelude.n_pos_args + prelude.n_kwonly_args; ++i) {
	mp_obj_t o = (mp_obj_t)*const_table++;
	save_qstr(print, MP_OBJ_QSTR_VALUE(o));
	}
	for (uint i = 0; i < rc->data.u_byte.n_obj; ++i) {
	save_obj(print, (mp_obj_t)*const_table++);
	}
	for (uint i = 0; i < rc->data.u_byte.n_raw_code; ++i) {
	save_raw_code(print, (mp_raw_code_t)(uintptr_t)const_table++);
	}
	}

	void mp_raw_code_save(mp_raw_code_t rc, mp_print_t print) {
	// header contains:
	// byte 'M'
	// byte version
	// byte feature flags
	// byte number of bits in a small int
	byte header[4] = {'M', MPY_VERSION, MPY_FEATURE_FLAGS_DYNAMIC,
	#if MICROPY_DYNAMIC_COMPILER
	mp_dynamic_compiler.small_int_bits,
	#else
	mp_small_int_bits(),
	#endif
	};
	mp_print_bytes(print, header, sizeof(header));

	save_raw_code(print, rc);
	}

	typedef void (mp_print_strn_t)(void data, const char *str, size_t len);

	typedef struct _mp_print_t {
	void *data;
	mp_print_strn_t print_strn;
	} mp_print_t;

persistentcode.c: save_raw_code() - add flush/finalizer callback #3478

Description

Activity

dpgeorge commented on Dec 13, 2017

adritium commented on Dec 20, 2017

dpgeorge commented on Dec 22, 2017

adritium commented on Dec 26, 2017

dpgeorge commented on Dec 28, 2017

dpgeorge commented on May 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions